Morgan Wilde
Morgan Wilde

Reputation: 17323

What would be the idiomatic C way to format this statement

I've come up with this hard to read if statement, and I can't figure out the best way I should format it's layout to make it more readable and do so in a C way.

if (((value == intMax) && (intMax != 0)) || // Deals with upper bound
    (value > (intMax/10)) ||
    ((value == (intMax/10)) && (digitAdjusted > digitLastIntMax)) ||
    ((value == intMin) && (intMin != 0)) || // Deals with lower bound
    (value < (intMin/10)) ||
    ((value == (intMin/10)) && (digitAdjusted < digitLastIntMin))) {

    // Some code
}

If this is an inappropriate use of SO, please let me know - I'll remove this question.

Upvotes: 2

Views: 91

Answers (5)

Morgan Wilde
Morgan Wilde

Reputation: 17323

So this is what I've come up with following all the advice that I've received.

// Upper bound
maxReached              = (value == intMax) && (intMax != 0);
maxDivTenExceeded       = value > (intMax/10);
maxLastDigitExceeded    = (value == (intMax/10)) && (digitAdjusted > digitLastIntMax);
// Lower bound
minReached              = (value == intMin) && (intMin != 0);
minDivTenExceeded       = value < (intMin/10);
minLastDigitExceeded    = (value == (intMin/10)) && (digitAdjusted < digitLastIntMin);
// Stop conditions
stopMax                 = maxReached || maxDivTenExceeded || maxLastDigitExceeded;
stopMin                 = minReached || minDivTenExceeded || minLastDigitExceeded;


// Prevent integer overflow
if (stopMax || stopMin) {
    // Code
}

Upvotes: 3

user3386109
user3386109

Reputation: 34839

Lining things up vertically helps a little bit, but the real question is, "What are the requirements that you're trying to implement?". It sort of looks like your converting an input string to an integer while trying to avoid overflow, but the intMin checks make no sense in that case. A little more context might help to sort this out.

if (((value == intMax) && (intMax != 0)) || (value > (intMax/10)) || ((value == (intMax/10)) && (digitAdjusted > digitLastIntMax)) ||
    ((value == intMin) && (intMin != 0)) || (value < (intMin/10)) || ((value == (intMin/10)) && (digitAdjusted < digitLastIntMin)) )

Note that vertical alignment would have made the bitwise OR vs. logical OR errors immediately obvious.

Upvotes: 1

user3407746
user3407746

Reputation: 26

If you want to make it more readable, use nested ifs and a good explanation in comments as to what each stage is doing. There's not much practical benefit to doing it all on one line and it certainly does seem unreadable.

Upvotes: 1

Benesh
Benesh

Reputation: 3428

Actually, since && precedes ||, the inner parenthesis aren't necessary - you can remove them.

Also: (intMin != 0) is the same as just intMax.

if ((value == intMax) && intMax || // Deals with upper bound
    (value > intMax/10) ||
    (value == intMax/10) && (digitAdjusted > digitLastIntMax) ||
    (value == intMin) && intMin || // Deals with lower bound
    (value < intMin/10) ||
    (value == intMin/10) && (digitAdjusted < digitLastIntMin)) {

    // Some code
}

Upvotes: 4

R Sahu
R Sahu

Reputation: 206697

I would put each of the clauses separated by || into functions that return a boolean value.

if ( test1(value, intMax) ||  // ((value == intMax) && (intMax != 0)) || // Deals with upper bound
     test2(value, intMax) ||  // (value > (intMax/10)) ||
     test3(value, intmax, digitAdjusted, digitLastIntMax) || // ((value == (intMax/10)) && (digitAdjusted > digitLastIntMax)) |
     test4(value, intMin) //     ((value == intMin) && (intMin != 0)) || // Deals with lower bound
     test5(value, intMin) || // (value < (intMin/10)) ||
     test6(value, intMin, digitAdjusted, digitLastIntMin) //((value == (intMin/10)) && (digitAdjusted < digitLastIntMin))
)
{
// Some code
}

Upvotes: 2

Related Questions