You sit on a solution to a problem for several days. You come up with its solution. You design classes and methods. You're immensely proud of yourself... and suddenly you find out that you've named variables wrong, methods are unintuitive, the whole class to be rewritten, and in general you didn't fully understand the initial problem anyway. How to avoid a collision with the brutal reality, and how to objectively review the code of others? This article will give you a hint.
What exactly is code review and what is it for? Code review usually takes place after the implementation of a given functionality, at which time the other team members decide whether to implement the changes in the project or whether they still need some improvements. Comments and suggestions on code review can be divided into 3 levels:
Level 1 - TYPOs, typos and minor comments.
This is the most basic type of comments, the easiest for reviewers to spot.
As you can see from the example above, the addd method should be changed to add. Detail over detail, however, such comments are also needed to keep the code clean. Suggestions we know the names are also needed:
The method converts a collection of names - they should have at least 3 and (acct in this particular case) at most 30 letters, then converts all letters to lowercase. What can be changed here? Well, the name of the method, although explaining its implementation, is quite long, you can think of a better one:
The method name has been shortened, moreover, it does not reveal to the outside world the implementation details - the method "knows" what length the names should be - someone who calls the method does not need to know these details. The local variables describing the numbers 3 and 30 have also been extracted - they are no longer so-called "magic numbers". Notes of this style are also necessary as much as possible, but we should not limit ourselves to them only.
Level 2 - Structure of classes and methods, tests, design patterns
Adding this type of remark already requires a longer and more in-depth analysis. Let's consider again the example from the previous type of remarks:
Level 2 comments could be as follows:
- Should the names collection be a field, or should it nevertheless be given as a method argument?
- Should the names collection become a list, or would a set be a more appropriate form for it, however? Do we accept duplicates?
- Should the applicableNamesLowercased method remain public? Perhaps we only call it inside a class, or in subclasses?
- Have tests been written for the above method?
Going back to the initial example with the calculator:
- Shouldn't the class be marked as final? After all, we are unlikely to extend the calculator class.
- Shouldn't the add method be static? If the whole class is an util style class?
- Or should the add method remain non-static after all, since static methods are more difficult to test?
- Have tests been written for the above method?
As we can see, there are many more considerations that qualify for Level 2 - one can even say that in some nuances, as many people as many opinions. However, they should all have a common denominator - striving for the most readable code possible.
Level 3 - Business logic
To add a note of this kind, you need to know the project you are working in well, preferably holistically, not just a piece of functionality. Although sometimes common sense and a fair reading of the code is enough. Let's look at the method that has already appeared to us:
The minimum length has been changed! A programmer who does not fully understand the initial assumptions (minimum 3 letters) will write functionality that "works", even tested, and the tests pass... However, that programmer wrote the tests based on a false assumption! Level 3 style comments are highly valued by other programmers.
We have already learned about the different types of comments in code review. But what about the eternal wars (bars vs. spaces) or the different opinions of two programmers? Let's remember a very important word - CONVENTION. Do you think your colleague has named a class incorrectly? Pay attention first to whether he followed convention, that is, the rules adopted (by the team). Even "not-quite-clean code" in a large project, if kept to convention, is much more readable than "clean code" where each module has a different approach.
Let's also be open to substantive criticism from others - instead of "he's stuck again," think of the comments on code review as an opportunity for self-development and noticing other forms of vision. Every programmer feels satisfaction with his code, it's natural and healthy (it would be bad if there was no such satisfaction). However, let's not get too emotionally attached to one's solution. In the other direction, let's also not "take it out" on our teammates ourselves, exalting ourselves when we find some mistake in their code. Let's remember that code review has a common goal - to deliver quality code.
