loading...

Don't let programmers alone

donut87 profile image Christian Baer Updated on ・2 min read

Recently I discovered a small piece of code that looked like this:

function isElementSpecial(htmlElementToBeChecked) {
  var result
  if(document.querySelector(htmlElementToBeChecked).classList.contains('special')) {
    result = true
  } else {
    result = false
  }
  return result
}

I cringed.
My first thought after my initial shock was: 'What on earth was this developer thinking while writing this?' I had to look at this a couple of times to make sure that the above code really only does one thing. Checking that an HTML element has the class special.
Since I had no time to investigate the how and why this code came to life, I did what every programmer would have done and quickly condensed the code to its essential core.

function isElementSpecial(htmlElementToBeChecked) {
  return document.querySelector(htmlElementToBeChecked).classList.contains('special')
}

Since I know whom amongst my colleagues did write this code, I was tempted to ask him straight away why he was writing a simple check in such a complicated manner. Luckily he had already gone home. So I had some minutes to think about what went wrong.
Some facts

  1. The code is from our front-end test system
  2. The responsible colleague is a senior programmer with some experience in that field
  3. He built our testing framework on top of CasperJS by himself
  4. He has his hands full with keeping our front-end tests stable

Look at point 3. He built this by himself!
There was no one to review, question or criticize his code.
In my opinion, this could have been prevented by someone reviewing his changes in regular and short intervals.
We as programmers build complex (and often large) systems. Having a complete in-depth knowledge of all lines of code is impossible, even if one single person wrote them all. Consequently, it is only natural to forget about that one line or method you wanted to refactor or rewrite. Also, there is a higher risk of getting attached to your code. Why this is a bad idea is excellently explained here.
There are only two methods known to me to reduce all of this. The first one is pair programming and the second one is code reviews. If for whatever reason pair programming is not an option (e.g. distributed team), that leaves only code review.
There are many good 'manuals' for the dos and don'ts of good code reviews (e.g. This).
At my workplace, we do team code review, although most of our code is done while pairing. Every morning we ask ourselves 'do we have something to show the team' and then the projector is turned on and we have a good half an hour of looking at code diffs and asking questions. This often enough reveals useless comments, ambiguous variable/method names and other code styling issues. We also have a general idea of the code that our team produces and our bus factor is increased drastically.
In short: Do code reviews and do them often. There is a high chance of making your code understandable, increasing the bus factor and reducing the attachment to the code.

Discussion

pic
Editor guide
Collapse
giuunit profile image
Giu

I do understand the idea of this article - code reviews are the best to avoid bad code. The idea behind the article is 100% right.

However I feel like there could be a better example than this code. This one doesn't have an error in it, it's just incredibly long for what it is doing.

Collapse
donut87 profile image
Christian Baer Author

Thanks for your comment. I totally understand your objection, but I have my reasons for choosing exactly this example.
My first point was, that I had to triple check if this code really does only one thing. It is not just incredibly long, it is redundant. Most of our developer time goes to reading code. So it should be easy to understand.
My second point was, that I knew exactly, that the author of these lines is no rookie. So for me, this was not redundant code at first, it was treacherous.