Code reviews. What? Why? How?

We all heard about code reviews… We all read articles about how to make a good code review and what’s the reason for doing it. Most of us had a few discussions regarding the benefits and pros and cons of code reviews at least with some managers.

My reasons were:

• Get a faster feedback
• Knowledge sharing (both from a technical and the business logic point of view)
• Increase productivity (since everyone knows at least half of the project, not only the part on which they worked).
• Everyone to know, respect and use the same standardization
• Keep the quality of the project at the level accepted and approved by the team
• Spot errors/bugs before going into the testing phase

Well…a few months back I needed to make a checklist for a new team, with things each member should check while doing a code review. From the checklist a ended up creating more like a list of guidelines, but who cares as long as the entire team agrees and are happy with the result?

Most probably, you dear reader don’t really care, about my story, or my opinion, since you already read a lot on the subject, but you need a list of things to look over for your team, so most probably what I’ll write below it will help you in the end.

Checklist/Guidelines

• Small smoke test / functional test (to make sure nothing obvious was broken). Usually, this is done in the dev environment. I know code review means the review of the code, but it won’t be nice how the tester will look at you after you made your code review and gave your thumbs up and he won’t even be able to log in to the application, even if the change was regarding a profile picture.
• Tests that have a high value were written — unit and integration tests. (Sorry, I don’t believe in code for production, without unit and integration tests. I accept the idea of creating something without tests, only if it will be used only once and after that, it will be burned with Greek fire, or create something for someone who you won’t ever meet again and no one from your acquaintances will need to maintain/work on it). I think if you are (or consider yourself) a professional developer you shouldn’t accept the idea of writing code without tests.
• In case of a bug fix, check if tests were written so that the bug won’t appear ever again
• Check the code coverage and be sure it’s accordingly with the minimum level accepted by the team. (Code coverage…line coverage…block coverage…even mutation tests coverage, this should be numbers agreed by the team and only for the team. Most managers don’t get them anyway, and you’ll lose a lot of time only to explain what mutation tests are, or why your code coverage isn’t 100%).
• Check the naming of the variables and methods (check readability, BE SURE that the method is doing what its name says)
• Check that public interfaces have well-written comments. (Here, everyone has an opinion, but I personally really read the summary of a method from its contract if I have no clue what it does, so for me, this is a plus)
• Check that APIs have well-written documentation. In Java, you’ve got the Javadocs, in.Net5 you have Swagger added by default in the Webapi template, in Kotlin you have Dokka and so on.
• Check if everything is clear (and it’s working) in Swagger or Postman in the case of APIs. (If you have a WebApi and you don’t have either installed, you should add a new item into your backlog as technical debt)
• Check not to have duplicated code. Two times to have a part of your logic duplicated, let’s say we can live with that. If the third time you’ve got the same thing written, well…you should do something about it!
• Check the number of parameters and number of lines (clean code rules: 5 parameters at most, 50 lines of code)
• Check for defensive coding (when and where it is the case)
• Check that the newly written code is not breaking your architecture (check references). This can be done using build rules, but probably dear reader you won’t get time to invest in creating them.
• Check that the code complexity isn’t too high ( do not over complicate things / over-engineering e.g. Generics shouldn’t be used if they aren’t really necessary).
• Check that the new 3rd party libraries added are really needed and used Be careful: do not add a 3rd party stuff only for one small thing. You might end up building your puzzle using A LOT of dependencies and it’s not nice.
• Check that no rules accepted by the team were suppressed. By rules I mean Stylecop rules, SonarQube
• “Boy scout rule” — If someone changed something in a file check if he tried to make at least 2 lines better than they were before he started writing his code.
• Before making a merge or sending your item in the “next step”, check that it is not an opened discussion regarding the changes.
• Check for points of failure in the code, corner cases that are not covered by a guard.
• Check if there are any optimization and security improvements that can be done and were not implemented.
• Check if there might be any deadlocks/ race conditions that might cause issues.
• Check that the code is solving the current issues from the story, not some things that might appear in the future. (YAGNI)
• SOLID and the other best practice principles are applied in most cases.
• Zombie code should ALWAYS be removed.
• No TODOs or other not useful comments are left in the MR.
• If there is any technical documentation written in a wiki or a confluence page, check if it’s still relevant. False documentation & a bad one is worse than no documentation at all. (I have a colleague that is saying me this quote, at least once a month. )
• If there are UI changes to be made, check they are made accordingly to the design created by our UX.
• Check the warnings and Infos.
• Check if methods aren’t “more public” than they should be.
• Check if there are constants and readonlys where they can be used. Don’t give the opportunity to someone else to break the behaviour of your objects.
• Static and hardcoded values should be used only when they are really necessary.
• Check that “no more than it’s used it is given”.

Other things to consider:

• No one wants to write bad code. If someone writes bad code he probably doesn’t know how to write a better one. Don’t get frustrated about this
• Don’t make any comment on an MR/PR personally! No one is criticizing you! We should do code reviews…so it’s about the code, not the coder!
• On code reviews do 121s or meetings, ask clearly when you’ve got a question and once you put a comment have all the arguments ready to support your idea. There are no good or wrongs, only different opinions (alternatives), so try to learn something from each comment and code review. (“There is no such thing as “perfect” code — there is only better code”, source → Google )
• If a code review has more than 3 comments a call/audio discussion might help to get the answers faster.
• Personal preferences or software styling shouldn’t be added more than just a remark.
• Almost every time a better name can be found!
• Never give a comment about something being not ok, without giving a suggestion. If you don’t have a better/improved way, probably what the developer wrote is the best solution. Of course, this can be a good subject of discussion but that’s all.
• We should let the architecture and design evolve
• A code review should take 5%-10% of the development time (15% tops when talking about algorithms). “Don’t rush perfection!”. Of course, if a code review is done by someone who was present in most part of the development (pair programming) the code review might be finished faster.
• A code review should not stay on the board without being started for more than 8 hours. It should have a higher priority than an implementation.
• Reinventing the wheel is a bad thing!

A lot of other things may be added and most likely I’ve missed some things, but in my opinion, this is a good start for a project.