Home ›

basic rules for code readability and the if statement

Christian Harms's picture
by Christian Harms on Mon, 08/01/2011 - 17:55
in

Some reader knows my preference for programming languages like python or javascript. One reason is the readability (yes, this is possible with many languages except brainfuck or whitespace). Once we write some lines of code, but other developer has to read the lines of code several times while code reviews, debugging or refactoring.

Code should be readable like your normal language. I will start with a funny condition.

  1. if ( !notOk != false ) {
  2.   userObj.ask();
  3. }

This is only confusing and you will never formulate this expression in a natural language. With several steps this term can be simply resolved:

  • ( !notOk != false )
  • ( !notOk == true )
  • ( !notOk)
  • now you should rethink the variable name: isOk = !notOk

And the result is more readable:

  1. if ( isOk ) {
  2.   userObj.ask();
  3. }

if - if - if trees

After removing some comments and other code lines this is an other typical coding style:

  1. if ( A ) {
  2.   if ( !B ) {
  3.     if ( C ) {
  4.       f();
  5.     }
  6.   }
  7. }

There shorter variant for clever coder looks like this line:

  1. if ( A ) if ( !B ) if ( C ) { f(); }

But using logical operators the line is more readable:

  1. if ( A && !B && C ) { f(); }

If you refactor a function with some levels of if-statements try to use logical operators instead of the if - trees. But it should be readable like a natural language.

Be careful while using & and/or comparing integer! Single & is in most languages the bit-wise AND operator, check the examples:

  1. ANSI-C:
  2. 1 & 2 = 0
  3. 1 && 2 = 1 //it’s true, and true is 1
  4.  
  5. JavaScript:
  6. 1 & 2 = 0
  7. 1 && 2 = 2 // the first is true, so return the second
  8.  
  9. python:
  10. 1 & 2 = 0
  11. 1 and 2 = 2 # like the JavaScript variant
  12.  
  13. Java:
  14. 1 & 2 = 0
  15. 1 && 2 // not valid with int

The if - true - false assignment function

  1. function isNameSet(name) {
  2.   if (name===null || name===””) {
  3.     return false;
  4.   } else {
  5.     return true;
  6. }

Sometimes this construct is hidden in many lines of code and can found in other languages too. But in this simple form the more elegant solution is easy to understand:

  1. function isNameSet(name) {
  2.   return !(name==null || name==””);
  3. }

But you only want to check it the value of name is set. Many languages like Javascript (read the all about types by Matthias Reuter) can convert the value directly into a truthy value (read more on The Elements of JavaScript Style by Douglas Crockford.

  1. function isNameSet(name) {
  2.   return name;
  3. }

After reducing the function to nothing you can remove it and use the string-value directly in a condition.

  1. if (!name) {
  2.   name = window.prompt(“What’s your name?”);
  3. }

use the ternary operator (carefully)

  1. function genderStr(gender) {
  2.   if ( gender == “m” ) {
  3.     return “male”;
  4.   } else {
  5.     return “female”;
  6.   }
  7. }

The function can be generalized:

  1. function trueFalseStr(cond, trueStr, falseStr) {
  2.   if (cond) {
  3.     return trueStr;
  4.   } else {
  5.     return falseStr;
  6.   }
  7. }

Or you can use the ternary operator (avaible in many languages):

  1. return (gender == “m”)? “male” : “female”

With python (since v2.5) the following term is available instead of the “?” operator:

  1. return “male” if gender == “m” else “female”

An other variant works with the example values and is readable too:

  1. return (gender==”m”) and “male” or “female”

yoda conditions

I offered in a older arctile the helpful yoda conditions. This simple example show the difference:

  1. if (value = 42) {
  2.    ...
  3. }

For preventing assignment in a condition (and get a compiler warning) use the constant first:
  1. if (42 = value) {
  2.    ...
  3. }

I have to withdraw in the content of readablitiy my suggestion for yoda conditions!

many if statements for mapping values

I found a nice thread on stackoverflow.com:

  1. function norm(v) {
  2.   size = 7;
  3.   if ( v > 10 ) size = 6;
  4.   if ( v > 22 ) size = 5;
  5.   if ( v > 51 ) size = 4;
  6.   if ( v > 68 ) size = 3;
  7.   if ( v > 117 ) size = 2;
  8.   if ( v > 145 ) size = 1;
  9.   return size;
  10. }

There are many variants, the one with JavaScript with ternary operators is not readable:

  1. function norm(v) {
  2.   return return v > 145 ? 1 : v > 117 ? 2 : v > 68 ? 3 : v > 51 ? 4 : v > 22 ? 5 : v > 10 ? 6 : 7;
  3. }

Or the calculated variant in ANSI C. It works only if a boolean value can converted to integer.

  1. int norm(int x){
  2.   return 7 - (x>10) - (x>22) - (x>51) - (x>68) - (x>117) - (x>145);
  3. }

If would change the limits for a more readable variant (in python):

  1. def norm(v):
  2.     if   v< 11: size=7
  3.     elif v< 23: size=6
  4.     elif v< 52: size=5
  5.     elif v< 69: size=4
  6.     elif v<118: size=3
  7.     elif v<146: size=2
  8.     else: size=1
  9.     return size

And now the variant can written as a list comprehension (ok, not better readable, but funny):

  1. def norm(v):
  2.     return 1+len([x for x in [11, 23, 52, 69, 118, 146] if v<x])

my conclusion

Try to read your code as a story. It should have an introduction and then things should happen step-by-step. For deeper information try to learn more about clean code and check out clean code books on amazon.

Comments

Anonymous's picture
Error in if-else example
Anonymous (not verified) - Mon, 08/01/2011 - 14:46

Your second version of isNameSet returns the reverse sense from the others.

Christian Harms's picture
one character added
Christian Harms - Mon, 08/01/2011 - 14:49

*ups* the was a missing ! in the function - thanx!!!

Keith Thompson's picture
isNameSet
Keith Thompson (not verified) - Mon, 08/01/2011 - 14:49

In the second version of your isNameSet() function, you reversed the sense of the test, so it returns true if the name *isn't* set. You also replaced "===" by "==" (I don't know JavaScript well enough to understand the implications of that). It should be something like:

  1.     function isNameSet(name) {
  2.       return name !== null && name !== ””;
  3.     }

(I'm assuming here that "!==" is the opposite of "===", and that an empty name is considered "unset".)

Christian Harms's picture
other variant
Christian Harms - Mon, 08/01/2011 - 15:04

I should copy every version of the function into the article. Thanx - this is also correct.

David Ellis's picture
Mapping Values in JS
David Ellis (not verified) - Mon, 08/01/2011 - 15:23

For both legibility and compactness of code in JS, I would write the last example as follows:

  1. function norm(v) {
  2.     var out = null;
  3.     [145, 117, 68, 51, 22, 10, -Infinity].forEach(function(val, ind) {
  4.         if(!out && v > val) { out = ind+1; }
  5.     });
  6.     return out;
  7. }

If the performance of this function was a problem, first change it to a standard for(var i = 0; ...) loop with a break, then if still a problem, use its sorted nature to binary search the array, but those are both (in my eyes) less readable than the above code.

David Ellis's picture
Another thought
David Ellis (not verified) - Mon, 08/01/2011 - 17:10

Actually, if this sort of pattern shows up often in the source, it would probably make sense to generalize it:

  1. function range(rangeArr, val, action) {
  2.     //Assumes rangeArr is a sorted, least-to-greatest array
  3.     //where a range is defined as rangeArr[n] <= x < rangeArr[n+1]
  4.     for(var i = 1; i < rangeArr.length; i++) {
  5.         if(val < rangeArr[i] && val >= rangeArr[i-1]) {
  6.             break;
  7.         }
  8.     }
  9.     return action(i);
  10. }

Then norm can be defined as:

  1. function norm(v) {
  2.     return range([-Infinity, 11, 23, 52, 69, 118, 146], v, function(i) { return i; });
  3. }

This way the action to be taken can be changed as needed via the callback function (though in this case it aligns perfectly with the defined problem). I am assuming that you want as many ranges as are defined elements in the array. If that last implied range (146->Infinity) is not desired, the callback would have to throw an error for that value.

Jens Finkhäuser's picture
If and return
Jens Finkhäuser (not verified) - Mon, 08/01/2011 - 15:33

I think of readability as a highly contentious issue. While some constructs like your initial example of ( !notOk != false ) can easily be improved upon, others are more difficult to gauge.

When I was still working for your company, I ran into the problem that someone really disliked that I often used: multiple return statements per function. To me, the following is very readable:

  1. function f(param)
  2. {
  3.   if (null == param) {
  4.     return;
  5.   }
  6.  
  7.   if ("" == param) {
  8.     return;
  9.   }
  10.   // Do something.
  11. }

By contrast, this here requires more brainpower to understand (even more so if you don't use a simplified example as I do):

  1. function f(param)
  2. {
  3.   if (null != param && "" != param) {
  4.     // Do something.
  5.   }
  6.   // Single point of return
  7. }

There are reasonable reasons for either variant. The second version produces much fewer lines of code, which allows more of the context of whatever line of code has your attention to be on screen at the same time. The first version on the other hand makes the contract of the function abundantly clear.

I still strongly favour the first variant; to me it's a question of clarifying the code itself, rather than better fitting the code to whatever editor settings you prefer. YMMV.

Christian Harms's picture
multiple or single return
Christian Harms - Mon, 08/01/2011 - 17:47

Hi Jens, we both know the pro and cons are long discussed and mostly the "single return" option can be found in some style guide. Thanx for the long comment - see some comments on stackoverflow.com .

Nafees's picture
single-return-statement
Nafees (not verified) - Sat, 08/06/2011 - 08:52

I usually prefer single return and a template like below

  1. public Boolean isSomeTest(int toEvaluate) {
  2.         Boolean retValue = null;
  3.         if(/*some-condition*/) {
  4.                 retValue=true;
  5.         } else {
  6.                 retValue=false;
  7.         }
  8.         return retValue;
  9. }

Makes it clear all along the procedure that this field "retValue" is going to capture the return value and then finally it will be returned.