DevOps Zone is brought to you in partnership with:

I've been a software engineer for about 7 years now. In the beginning, I started my career in the financial industry in Wall Street, working on overly complex systems that almost made me lose interest in the software craft. A couple years later, I decided to move on to working with startups and haven't looked back since. I particularly, enjoy working with Redis and using it's simple datastructures to efficiently solve complex problems. Santosh is a DZone MVB and is not an employee of DZone and has posted 9 posts at DZone. You can read more from them at their website. View Full User Profile

Clean Code: Conditionals

02.17.2014
| 13706 views |
  • submit to reddit

This is part of a series of micro-blogs (somewhere between a tweet and a full on blog) on Clean Code. In this post, I’m going to focus on one big culprit in obfuscated, not clear clean code — Conditionals (i.e. if statements). Let’s look at an example:

function handleSearchRequests(req, res) {
    if (req.params['toggle'] === 'A') {
        doToggleSearch(req.params, res);
    } else {
        doRegularSearch(req.params, res);
    }
}

While this looks pretty straightforward, this is a code smell and is a broken pane waiting for the crack to grow bigger. What does this code actually convey? This is not entirely clear to anyone else, except probably the person who wrote it.

Quick Fix

Make all conditionals only use local variables that identify the role they are playing. So, in this case the code becomes:

function handleSearchRequests(req, res) {
    var isAToggleSearch = (req.params['toggle'] === 'A');
    if (isAToggleSearch) {
        doToggleSearch(req.params, res);
    } else {
        doRegularSearch(req.params, res);
    }
}

Here we are doing two things:

1. We are clearly identifying this concept of a toggleSearch and being very explicit about what is and is not a toggleSearch.
2. In our conditional we are making the branching more obvious and explicit.

TL; DR

By just going through this exercise of using local role playing variables in conditionals you’ll be surprised on just how much clearer your code becomes.



Published at DZone with permission of Santosh Kumar, author and DZone MVB. (source)

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Comments

Stephen Lindsey replied on Mon, 2014/02/17 - 5:12am

"What does this code actually convey? This is not entirely clear to anyone else, except probably the person who wrote it."

Sorry but I think any programmer would follow the logic, maybe not the reasoning behind the logic - but there again the second implementation adds nothing, now if the "A" flag represented,say, "area" and you called your variable isAreaSearch....

Andre Bogus replied on Mon, 2014/02/17 - 7:03am in response to: Stephen Lindsey

I wholeheartedly agree. The extra name just restated the logic of the if-condition, thus added no value. Without more context it is difficult to say how the code could be improved. Even so, the code could be improved tremendously by adding a comment that describes the rationale for splitting the search function.

"There are two hard things in programming: Naming things, cache invalidation and off-by-one errors." -- origin unsure

Jamie Bisotti replied on Mon, 2014/02/17 - 9:03am

I disagree with the above two commenters. Though the author's given example might be too simple; the idea is a very good one.

What if it had been: 

req.params['t'] === 'A'

Using 't' in place of 'toggle' makes it less obvious and the well-named local variable would be even more useful.

Clean code, as espoused by Uncle Bob (as per my understanding), prefers well-named variables/methods over comments. And it prefers explicit names for ideas/concepts/logic over code that can be reasonably deciphered. Though it would likely be possible for a developer to follow a given chunk of not-so-clean-code, doing so takes time and it takes that time every time that piece of code is looked at. Clean code tells the story explicitly; thus, it does not need to be deciphered; thus, it is easier to understand and maintain.


My $0.02.

andy darlow replied on Mon, 2014/02/17 - 11:10am

 The other boys are on the mark. Also, the refactored line is still at a lower level abstraction to the other lines. So, maybe it's better to seperate out that line so that people can read the code without knowing the gory details i.e.



function handleSearchRequests(req, res) {
   return  userSelectedToggleSearch(req) ?
         doToggleSearch(req.params, res) :
         doRegularSearch(req.params, res);
  }

function userSelectedToggleSearch(req) {
  return  (req.params['toggle'] === 'A');
}


Andy

Stephen Lindsey replied on Mon, 2014/02/17 - 9:40am

You say that you  disagree with what I say but then go on to write " prefers explicit names" which is what I suggest. Nowhere in the old or new code do we know what a toggle search is.

The OP idea is not bad at all just, as i point out, incomplete.

The remark about t instead of toggle is a red herring as the interface is what it is.

But we're all entitled to our $0.02

Peter Verhas replied on Mon, 2014/02/17 - 10:57am

I agree with the first two commenters. There is no added value in the extra variable. Even though the pattern can be helpful if

1. the code is more complex than just a simple if/then/else or if the conditional is very complex and not easy to understand just looking at it

2. if there is some extra information in the name you just can not distill from the code

In the example none of the above is the case which makes the example less than perfect and may even direct the oblivious junior into wrong directions.

David Vree replied on Wed, 2014/02/19 - 1:36am

I like the added commentary that the variable name supplies.  It also easier to debug this way because you can stop after the variable assignment and see whats up.

Oleksandr Alesinskyy replied on Thu, 2014/02/20 - 6:53am

-- Removed as duplicate --

Oleksandr Alesinskyy replied on Thu, 2014/02/20 - 6:51am

 While this approach may be useful sometimes, the example is made definitely much less readable/understandable by the "improvement".

Daniel U. Thibault replied on Thu, 2014/02/20 - 8:54am

 I agree with the first posters that this tip is nearly pointless.  It does nothing that good Javadoc wouldn't do better.

Pierre-yves Saumont replied on Thu, 2014/02/20 - 9:32am

Noise. Nothing else but noise. From reading the code, we understand immediately that the result of the expression is the condition tested in the if construct. It adds absolutely nothing to name it. The name is given by the method names. What if later you refactor the code from elsewhere changing the doToggleSearch method into doDuckDuckSearch (a very common refactoring with modern IDEs)? You end with:

isAToggleSearch = (req.params['toggle'] === 'A');
if(isAToggleSearch) {
  doDuckDuckSearch(req.params, res);
} else {
  doRegularSearch(req.params, res);
}

The problem with names is that they often lie. Without the local variable, it is crystal clear that the expression

req.params['toggle'] === 'A'

returns a boolean that is used for the conditional test. Why would you ever need to name it?

By the way, the real problem is not the name of the expression/condition. The problem is what are "req" and "res". I might infer that they are a request and a response. But what are their types? Strings ? Request and Response?.

We do not need names to write clean code. We need types. If you want to write clean code, use a statically typed language. I mean real useful types like Response, Request and SearchType, not String and Boolean). Something like:

def xyz(x: Request, y: Response): SeachResult = {
  val z: Search = SearchType(x.params['toggle'])
  z match {
    t: ToggleSearch => 
      toggleSearch(x.params);
    t: RegularSearch =>
      regularSearch(x.params);
  }
}

Plus, if you want to write clean code, do not write "functions" with side effects, but separate functions and effects.

If you think don't need types, you don't need names either. Life may be much more fun without types. But code will never be "clean" just by adding names.

Oleksandr Alesinskyy replied on Thu, 2014/02/20 - 9:36am in response to: Daniel U. Thibault

 The only thing is that the example is not in Java - who knows, if Javadoc is applicable to it :)

Daniel U. Thibault replied on Thu, 2014/02/20 - 10:33am in response to: Oleksandr Alesinskyy

 lol, right.  Proper code in any language should be documented Javadoc-style.

Pierre-yves Saumont replied on Thu, 2014/02/20 - 10:55am in response to: Oleksandr Alesinskyy

Of course it's not Java. The initial example is dynamically typed (Javascript?). My example, is no specific language, but statically typed. (May be Scala, or very close.) When I say that one need to use a statically typed language to write clean code, I mean a language with a decent type system.  Clearly not Java!

And by the way, why would clean code in a statically typed language, with a decent type system and a smart type model ever need Javadoc comments? (Or any other kind of comments, BTW!)

Robert Brown replied on Thu, 2014/02/20 - 11:02am in response to: Daniel U. Thibault

Personally, I think that if code requires commenting, it probably means it could be rewritten to better express the actual functionality.  Or it's tackling a very unorthodox problem.  Code should, in almost every case, be self documenting as to it's functionality.  Usage or API on the other hand is better expressed in comments.

I like the variable expression as a documentation step that is code based, so it will have the greatest chance of changing as the functionality changes.  Javadocs very often don't change when functionality changes unfortunately. 

I had a similar discussion with another developer where he was saying that a confusing code pattern that he used all the time just needed to be documented everywhere.  It was a javascript project where he was wrapping all the parameters for all functions into a temp object and passing it in instead of parameters.  So instead of  

doAJigWithMusic:function(music, volume){

}

a.doAJigWithMusic:function(myMusic, 23);

He had 
doAJigWithMusic:function(params){

}
var params = {
  music:myMusic,
  volume:23
}
doAJigWithMusic(params);

With a copious amount of documentation to identify what the possible param values could be.  
I said "what happens when the dev doesn't update the docs" and he said "Well you have to update the docs" at which point I pulled up a section of his code and said "Then why didn't you update the docs?"...

Even people who are extremely finicky about documenting miss it at times.  I would prefer to use a code structure that is clear.  

I'm not by any means saying that documenting your code is not useful or good, but only document when the algorithm is doing something out of the ordinary, or document the use cases for an API.  Documenting for the sake of documenting increases confusion as the code gets older because the documentation doesn't keep up with the changes to the code. 


Oleksandr Alesinskyy replied on Thu, 2014/02/20 - 11:15am in response to: Pierre-yves Saumont

First of all, Javadoc comments are not for developers of the code, they are for it users (which may not have an access to source or just do not want to dig into source).

Secondly, you always need comments to express your intentions - code expresses "how", to much lesser extent "what" and almost never "what for" and "why". But the last two are even more important than the first two.

Josh Hull replied on Thu, 2014/02/20 - 11:48am

I agree with you, Robert.  

That is right inline with principle 9 at this dzone article: best best practices ever  

Document design and purpose, not mechanics.

      1. document interfaces and reasons, not implementations
      2. refactor code instead of explaining how it works
      3. embed the documentation for a piece of software in that software

I think the author is trying to get us to #2, even if his refactoring doesn't get all the way there.   

Andy Darlow's comment takes it a step further by separating the variable evaluation into a method. That solves most of the issues people are bringing up, and makes it clear that further refactoring would likely be productive. 


Also, tip of the hat to Pierre:  separating code into algorithms and coordinators (which is what I'm interpreting your statement of "functions and effects") would clean this up further.   Another good dzone read There are only two roles in code 

Pierre-yves Saumont replied on Thu, 2014/02/20 - 12:01pm in response to: Oleksandr Alesinskyy

 Javadoc comments are very useful indeed. But how many times did you find such comment:

  /**
   * Gets the time when the agent connected.
   *
   * @return The connection time in milliseconds.
   * @since 1.0
   */
  long getConnectionTime();

The only useful thing here is that a wrong type is used and this error is documented in the Javadoc. A time should be of type Time, not long.

But this is not documenting code. The information you find in the Javadoc comes from the specification. It does not document the implementation.

The code shows "how". And types show "what". "what for" and "why" belong to the specification.

Pierre-yves Saumont replied on Thu, 2014/02/20 - 12:14pm in response to: Josh Hull

Josh: I did not invent it. This is just functional programing. And there is no need to use a special language for this. Functional programing principles may be used in any language. But with more or less pain. It's not a question of having "functions":

  • Don't mix returning a value and having a side effect in the same method
  • Design the write types
  • Use immutable objects
  • Respect referential integrity and referential transparency
  • Write "total" functions (methods) instead of partial ones
  • Insure that functions (methods) compose even when there is not result (null!) or an exception.
  • Implement true recursive methods (using the heap instead of the stack)
  • Implement lazy evaluation (difficult when not provided by the language!)
  • ...
All this is much more important if one wants to write clean code than using comments!

Daniel U. Thibault replied on Thu, 2014/02/20 - 4:28pm in response to: Oleksandr Alesinskyy

"First of all, Javadoc comments are not for developers of the code, they are for it users (which may not have an access to source or just do not want to dig into source)."

It is true Javadoc is mostly for the coding users, but I pity a developer who does not read the description of what he's about to code is supposed to do!  If you don't read the spec, how can you possibly code it right?



Oleksandr Alesinskyy replied on Thu, 2014/02/20 - 4:47pm in response to: Daniel U. Thibault

 "but I pity a developer who does not read the description of what he's about to code is supposed to do" - that's true, but such specifications quite often predate Javadoc, which is written from them.

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.