Code Perfectionist - Part 3 - One Liners

Written by Antony D'Andrea

I have been doing a little series of posts about my coding standards. Today will be the final part, I think.

Part 1 was about Spacing, and Part 2 about quotes. Today it is about code trying to be space efficient but ending up being unreadable.

I hate code that tries to be 'space efficient' and attempt to achieve multiple tasks in 1 line. For example, take this simple function:

public function simpleFunction($param)
{
    return (true === $this->someVar) ? $this->someVar : $this->anotherFunction($param);
}

Sure it looks 'clean'. Takes 4 lines of code (3 if you do the cardinal sin of putting the bracket on the same line as the function signature). But there are two issues:

a) Although it is one line, depending on the screen size or width of where the code is displayed, it gets either wrapped or goes off the screen and requires scrolling. Both of these reduce readability.

b) If, in the future, I need to alter this function to add more functionality to each condition, I will have to rewrite it.

So the function should be written like so:

public function simpleFunction($param)
{
    $someReturnVar = $this->someVar;
    if (false === $someReturnVar) {
         $someReturnVar = $this->anotherFunction($param);
    }
    return $someReturnVar;
}

Notice that it wasn't even necessary to have an else statement.

Also notice that the if statement still has bracket despite it only being one line. I hate:

if (false === $someReturnVar) $someReturnVar = $this->anotherFunction($param);

For the same reasons as above.

And even worse:

 if (false === $someReturnVar) 
         $someReturnVar = $this->anotherFunction($param);

This isn't Python!

Adding brackets makes it more readable and less prone to errors in the future when some clumsy developer comes along to add to it.

The only critique with the above solution is the fact that I give the same variable a different value. We know how loosely typed PHP is, so this could be potentially dangerous.

Another really annoying one liner:

 foreach($item in $list)if($item==true){

 }

And if that wasn't bad enough:

 foreach($item in $list)if($item==true){

 } else $this->somefunction();

And God help you if you have them nested:

 foreach($item in $list)if(!empty($item))foreach($i in $item)if($item==true){

      } else $this->somefunction();
 } else $this->somefunction();

I just don't understand what the above achieves. Someone who codes like that has a belief that they will never have to look at that piece of code and for some reason wants to cause pain to some poor developer who has to look at that.

It is almost impossible to see which if statement each of those elses go with.

The last one should be rewritten as:

foreach($item in $list) {
    if(!empty($item)) {
       foreach($i in $item){
           if($item==true){
              } else {
                $this->somefunction();
            }
       }
    } else {
        $this->foo();
    }
}

Ok, so it takes up more lines. If I have made a mistake in this, it only goes to prove how easy it is to misunderstand code that is written in one line.

It is so much more clear to see what it is doing, which code block goes with what and if I want to change or add to it, I can do so much more easierly. For example, I can now take out the nested foreach loop without any effort.

Please, NEVER try and be clever and 'clean' by missing out non-mandatory brackets or attempting to build in complex functionality into a single code.

This is not a competition of who can complete a task in the fewest number of lines. We are trying to write code that is not only correct, but is able to added to, modified or even reused in the future.

© 2012-2023