“if statements” are a “code smell”

(Everyday Code – instead of keeping our knowledge in a README.md let’s share it with the internet)

Today we made a code review on a feature. Generally the review process in our team is – someone drops a message in the slack channel and says – “hey, I have these changes. Can you please take a look at them?”. We do not require a merge from an “authority” to get something to production. We also have one rule we are trying to follow:

Your commit should make the product better than it previously was.

https://www.axlessoft.com/careers/

The commit below did not make the code better and this is the article about it. I am sure it would be useful for all of our team members and I hope it will be useful for the community as a whole.

How “if statements” are a sign of “code smell”

This is the commit. Do you see the problem with it?

   /**
    * @private
    * @param      {IS.StepsTree.LoadedEvent} event
    */
   onStepsTreeLoaded(event) {
-    this._buildId = this.generateBuildId();
+    if (!this._modeChangeOccured) {
+      this._buildId = this.generateBuildId();
+    } else {
+      this._modeChangeOccured = false;
+    }
+

The logic in the onStepsTreeLoaded method has significantly changed. It was a simple initialization of a private variable. Now it is an if statement with an else that sets the variable used in the if to a false value.

Wow. This is even difficult to explain.

Why was the change introduced and how the “code smell” helped us improve?

The thing is that our colleague had to do this change to keep the behavior of the code based on a commit from 7 months ago. But now we see this smelly code and we thought:

Why is this even needed in the first place? Why do we call this onStepsTreeLoaded method and what is it doing for us?

Turns out that we can just move the initialization from onStepsTreeLoaded method to another method called at a different place and we can delete this method. We would keep the same behavior. There will be no regressions. The framework has changed in the last few months to the point that there is now a better place for this initialization to happen.

My point is: “if statement”==”code smell”

Adding an if statement to a working code is probably a code smell. Wrapping an existing code in an if statement based on unrelated state with an else that sets this same state is probably the precise definition of what “code smell” is.

Conclusion

Revise your assumptions. Don’t add the if statements. Think again if this is really needed, why it is needed and where is its place in the architecture of the platform.

The concerned emotion

Can’t think of a better way to show you the emotion of code smell than to show you this Gorilla.

FabBRIX WWF, Gorilla in 3D building instructions