Should programmers use boolean variables to "document" their code?

I'm reading McConell's Code Complete, and he discusses using boolean variables to document your code. For example, instead of:

if((elementIndex < 0) || (MAX_ELEMENTS < elementIndex) || 
   (elementIndex == lastElementIndex)){
       ...
}

He suggests:

finished = ((elementIndex < 0) || (MAX_ELEMENTS < elementIndex));
repeatedEntry = (elementIndex == lastElementIndex);
if(finished || repeatedEntry){
   ...
}

This strikes me as logical, good practice, and very self-documenting. However, I'm hesitant to start using this technique regularly as I've almost never come across it; and perhaps it would be confusing just by virtue of being rare. However, my experience is not very vast yet, so I'm interested in hearing programmers' opinion of this technique, and I'd be curious to know if anyone uses this technique regularly or has seen it often when reading code. Is this a worthwhile convention/style/technique to adopt?开发者_如何转开发 Will other programmers understand and appreciate it, or consider it strange?

Splitting an expression that's too nested and complicated into simpler sub-expressions assigned to local variables, then put together again, is quite a common and popular technique -- quite independently of whether the sub-expressions and/or the overall expression are boolean or of just about any other type. With well-chosen names, a tasteful decomposition of this kind can increase readability, and a good compiler should have no trouble generating code that's equivalent to the original, complicated expression.

Some languages that don't have the concept of "assignment" per se, such as Haskell, even introduce specialized constructs to let you use the "give a name to a subexpression" technique (the where clause in Haskell) -- seems to bespeak some popularity for the technique in question!-)

I have used it, though normally wrapping boolean logic into a reusable method (if called from multiple locations).

It helps readability and when the logic changes, it only needs changing in one place.

Other will understand it and will not find it strange (except for those who only ever write thousand line functions, that is).

I try to do it wherever possible. Sure, you are using an "extra line" of code, but at the same time, you are describing why you are making a comparison of two values.

In your example, I look at the code, and ask myself "okay why is the person seeing the value is less than 0?" In the second you are clearly telling me that some processes have finished when this occurs. No guessing in the second one what your intent was.

The big one for me is when I see a method like: DoSomeMethod(true); Why is it automatically set to true? It's much more readable like

bool deleteOnCompletion = true;

DoSomeMethod(deleteOnCompletion);

The provided sample:

finished = ((elementIndex < 0) || (MAX_ELEMENTS < elementIndex));
repeatedEntry = (elementIndex == lastElementIndex);
if(finished || repeatedEntry){
   ...
}

Can also be rewritten to use methods, which improve readability and preserve the boolean logic (as Konrad pointed out):

if (IsFinished(elementIndex) || IsRepeatedEntry(elementIndex, lastElementIndex)){
   ...
}

...

private bool IsFinished(int elementIndex) {
    return ((elementIndex < 0) || (MAX_ELEMENTS < elementIndex));
}

private bool IsRepeatedEntry(int elementIndex, int lastElementIndex) {
    return (elementIndex == lastElementIndex);
}

It comes at a price, of course, which is two extra methods. If you do this a lot, it may make your code more readable, but your classes less transparent. But then again, you could also move the extra methods into helper classes.

Remember that this way you compute more than necessary. Due to taking conditions out from the code, you always compute both of them (no short circuit).

So that:

if((elementIndex < 0) || (MAX_ELEMENTS < elementIndex) || 
   (elementIndex == lastElementIndex)){
   ...
}

After the transformation becomes:

if((elementIndex < 0) || (MAX_ELEMENTS < elementIndex) |
   (elementIndex == lastElementIndex)){
   ...
}

Not an issue in most cases, but still in some it may mean worse performance or other issues, e.g. when in the 2nd expression you assume the 1st one has failed.

The only way I could see this going wrong is if the boolean fragment doesn't have a name that makes sense and a name is picked anyhow.

//No clue what the parts might mean.
if(price>0 && (customer.IsAlive || IsDay(Thursday)))

=>

first_condition = price>0
second_condition =customer.IsAlive || IsDay(Thursday)

//I'm still not enlightened.
if(first_condition && second_condition)

I point this out because it is commonplace to make rules like "comment all your code", "use named booleans for all if-criteria with more than 3 parts" only to get comments that are semantically empty of the following sort

i++; //increment i by adding 1 to i's previous value

By doing this

finished = ((elementIndex < 0) || (MAX_ELEMENTS < elementIndex));
repeatedEntry = (elementIndex == lastElementIndex);
if(finished || repeatedEntry){
   ...
}

you remove the logic from your brain and put it in the code. Now the program knows what you meant.
Whenever you name something you give it physical representation. It exists.
You can manipulate and reuse it.

You can even define the whole block as a predicate:

bool ElementBlahBlah? (elementIndex, lastElementIndex);

and do more stuff (later) in that function.

I think it's better to create functions/methods instead of temporary variables. This way readability is increased also because methods get shorter. Martin Fowler's book Refactoring has good advice for improving code quality. Refactorings related to your particular example are called "Replace Temp with Query" and "Extract Method".

Personally, I think that this is a great practice. It's impact on the execution of the code is minimal, but the clarity that it can provide, if used properly, is invaluable when it comes to maintaining the code later.

If the expression is complex then I either move it to another function which returns a bool eg, isAnEveningInThePubAGoodIdea(dayOfWeek, sizeOfWorkLoad, amountOfSpareCash) or reconsider the code so that such a complex expression isn't required.

if the method requires a notification of success: (examples in c#) I like to use the

bool success = false;

to start out. the code's a falure until I change it to:

success = true;

then at the end:

return success;

I think, it depends on what style you / your team prefer. "Introduce variable" refactoring could be useful, but sometimes not :)

And I should disagree with Kevin in his previous post. His example, I suppose, usable in case, when introduced variable can be changed, but introducing it only for one static boolean is useless, because we have parameter name in a method declaration, so why duplicate it in code?

for example:

void DoSomeMethod(boolean needDelete) { ... }

// useful
boolean deleteOnCompletion = true;
if ( someCondition ) {
    deleteOnCompletion = false;
}
DoSomeMethod(deleteOnCompletion);

// useless
boolean shouldNotDelete = false;
DoSomeMethod(shouldNotDelete);

In my experience, I've often returned to some old scripts and wondered 'what the hell was I thinking back then?'. For example:

Math.p = function Math_p(a) {
    var r = 1, b = [], m = Math;
    a = m.js.copy(arguments);
    while (a.length) {
        b = b.concat(a.shift());
    }
    while (b.length) {
        r *= b.shift();
    }
    return r;
};

which isn't as intuitive as:

/**
 * An extension to the Math object that accepts Arrays or Numbers
 * as an argument and returns the product of all numbers.
 * @param(Array) a A Number or an Array of numbers.
 * @return(Number) Returns the product of all numbers.
 */
Math.product = function Math_product(a) {
    var product = 1, numbers = [];
    a = argumentsToArray(arguments);
    while (a.length) {
        numbers = numbers.concat(a.shift());
    }
    while (numbers.length) {
        product *= numbers.shift();
    }
    return product;
};

I rarely create separate variables. What I do when the tests get complicated is nest the IFs and add comments. Like

boolean processElement=false;
if (elementIndex < 0) // Do we have a valid element?
{
  processElement=true;
}
else if (elementIndex==lastElementIndex) // Is it the one we want?
{
  processElement=true;
}
if (processElement)
...

The admitted flaw to this technique is that the next programmer who comes along may change the logic but not bother to update the comments. I guess that's a general problem, but I've had plenty of times I've seen a comment that says "validate customer id" and the next line is examining the part number or some such and I'm left to wonder where the customer id comes in.