Code review flavors

I have seen lots of people who are doing code reviews and others who think it’s just waste of time or they are not convinced about it. The latter are probably defensive and don’t want for others to tell them how good or bad is their code. And most probably they don’t write any kind of tests. They test it manually and it’s good enough from their point of view (after all we have QAs). Usually these are experienced guys who think they know enough. These guys are not professionals. You must accept constructive feedback and must not be defensive.

Benefits

If at a first glance code reviews seems to be time consuming, it’s actually not. It’s a tool of prevention. In the long run you save lots of time. Code reviewing time is coding time. It should be part of the development process and even part of definition of done(in a agile context). In order to do a successful code review the code must be relatively small so that you don’t get lost into details. If you will ask the developers to review 20 lines of code they will do a good job on it, but if that number grows to hundreds probably they will do a superficial job and approve the code.

Code review will improve your estimates. Everyone will be somewhat familiar with the zones their are not working on. Code reviews will force you to write better code as there is a pressure from the other team members. You don’t want to sent your code knowing that it’s not good enough. When you do code reviews you look for tests(unit or integration). That means code coverage should be improved as well. It’s a great way of collecting technical debt.

1-1 code reviews

It works as long as there are only two developers in a team. In any other case it does not work. I’ve seen it done by phone. One guy is calling the other one and shares his screen explaining what he had already changed. Yes he changed it before discussing with the code review creator. This is the worst type of review.

A better variant is to use the advantages of gitlab, bitbucket, github where you can actually use the comments section of the merge/pull request. You comment there and give the other guy the chance of responding. You exchange ideas and more importantly the creator of the merge request will change the code. Every comment must be resolved. When the situation requires it’s recommended to jump into a review call with the others, as it’s easier to communicate.

1-N code reviews

There is the concept in some companies that only senior guys are able to do code reviews. The have lots of experience and are appropriate to do it. Even this is miles better than the previous ones we can do better. Everyone must attend and contribute to the review. Doesn’t matter the level. All opinions must be listened, only this way a team can grow technically and you filter out even more the code, so the chances of introducing a bug decreases dramatically. Be graceful for every comment you receive as it’s a chance to improve. Don’t take it personally.

Code reviews it time consuming.  Sometimes it helps to checkout that branch and even run it. It’s a great tool of sharing ideas. Use it properly. Don’t be superficial about it.

No (manually) code reviews

Delegate this to static analysis tools like Qulice. The difference between this and regular code analyzers (Checkstyle, Cobertura) is that Qulice will agregate the information from these and even more it will break your build if the rules are not respected. How about that? Are you brave enough as a team to do this? It’s like getting 100% coverage. If you’re an Uncle Bob fan probably this is something to your liking. Even if you are not there yet you can still do code reviews with one of the tools that don’t break the build.

Summary

There must be some level on review on the code we write, the more reviewers(including tools) the better quality. I’m sure most of us can’t or won’t be super strict with the quality due to other restraints, like in outsourcing when it’s hard to convince the client that this will increase it’s costs on the short term, but decrease them on long term. They all said they want it, but after they realize how much it takes and sees that their product is not build at the pace they want, will silently drop it on ask that we do it superficially. After all money talks. It’s hard to find a client that will fully agree with this, even in product companies. That is if you’re not working for yourself. All I’m saying there must be some balance. If the code review takes too much time then a meeting is required to solve the issues.