Reputation: 17323
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
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
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
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
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
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