What is a good code review?

Every programmer does mistake in his code, whatever are his programming skills. You can detect these mistakes at different level in the development workflow : from the development itself to the production. What is sure is that the faster you will find a mistake, the faster you will correct it.

Reread the code, even if it works, is a good way to find potential bugs as fast as possible. This reading is called the code review.

Every team has his own way to do a code review. Some ways are bad, others are better. In this thread, I will present you what is for me the perfect code review, and we will see that a good code review has others benefits that just reveal potential bugs.

How?

The reader and the programmer must read the code together, side by side using the programmer computer. It encourages both of them to discuss and debate about the code in order to challenge it as most as possible.

The programmer must not introduce the modifications to the reader because the reader must understand himself the code. Indeed, the code must be clear enough because other teammates will have to work on it. So the reader must take the mouse and keyboard command.

To be sure to read every modified line of code, the review must be done from the modifications diff of the version control software. Of course, if necessary, the reader can have a look in the entire code.

If the reader see something which must be changed in the code, the programmer must take a note to don't forget to change it after the code review. Every modification must be done just after the code review.

What?

The reader has the responsibility to approve that the code is correct. If he doesn't see a bug during the code review, he will have the same responsibility than the programmer if a problem occurs in production. So every line of code must be checked and understood.

Several kind of problems can be detected in the code and fixed with the code review. For examples, the reader could do the following remarks to the programmer :

• "The format of these two lines of code doesn't respect our coding conventions"
• "When I read the method name, I don't understand what is his responsibility"
• "In this case, you will have a NullPointerException with this variable"
• "For this line of code, there already exists an utility method in the util package"
• "A unit test is missing for this case"
• "Here, you are doing a database access in a loop, you should refactor your code"
• ...

As you see, the code review allows to be sure that the code will respect the coding conventions, that the code will be understandable, and that potential bugs can be detected.

When?

When the development of a feature is finished, the developer has to demonstrate his work and to ask somebody to read his code.

If the developer starts with the code review and finishes with the demonstration, it is possible that he has to modify his code after the demonstration because the feature doesn't fit all the functional requirements. So a second code review will be necessary.

On the other side, if he starts with the demonstration and finishes with the code review, it is possible that he has to modify his code. But in this case, as the developer directly understood the functional requirements and wrote regression tests, it is not necessary to re-demonstrate the feature.

So the best way is to start to demonstrate the feature and to finish with the code review.

Who?

Who must read the developer code? The technical leader? A senior developer? A junior developer? Depending on the case, every teammate must be able to do a code review. Two parameters must be taken into account to decide who can read who : The first one is the technical level of both the developer and the reader. The second one is the criticality of the feature.

For a critical feature (something complex or really important for the business), the reader must be experimented, so either a senior developer or the technical leader.

For a "basic" feature, it is better to avoid to let a junior developer read another junior developer. Indeed they may don't know every coding conventions or miss bugs or performance issues. But a junior developer can read a senior developer : it is a good way for the junior to improve his competences.

Conclusion : benefits of a good code review

A good code review allows you to fix several bugs and to refactor your code really quickly. You have so the insurance to have a best quality in your software and in your code.

But more than that, encourage your team to read the code allows them to have a best knowledge of the code and to improve their competences very easily.

So if you think that this kind of code review is a waste of time, you are mistaken because you will have less bugs in production, a code of quality, and to finish a more competent team, so a more effective team.

Keep your code simple with YAGNI

One key principle of success for a web site is its capacity to always improve itself with new features and to do it faster than its competitors. To achieve that, the IT team must be able to create features as fast as possible at the different phases of the realization : development, tests and deployment.

During the development phase, one axis to work faster is to always have the more simple, readable and modifiable code. Besides several practices well known like code review or refactoring, there is an important principle which is not very well known by developers : YAGNI. Before explaining this principle, let's see how could a developer work without YAGNI.

Scenario without YAGNI

A agile development team has in its backlog several features and only the three first features are absolutely mandatory before the deployment in production :

• Creation of a user (must have)
• User listing (must have)
• Deletion of a user (must have)
• Modification of a user (nice to have)
• Search a user by login (nice to have)

A developer begins to work on the "Creation of a user example" feature. To do that he has to create the user interface, the business logic and the DAO layer.

When he creates the DAO layer, he adds a method "createUser" and then he directly creates several others methods : "getUsers", "deleteUser", "updateUser" and "searchUserByLogin". The developer did that because he is quite sure that these methods will be necessary for the four other features of the backlog.

public interface UserDao {
  public void createUser(User user);
  
  public void updateUser(User user);
  
  public void deleteUser(User user);
  
  public User searchUserByLogin(String login);
  
  public List getUsers();
}

Then specifications change

The three mandatory features are developed and pushed in production. 

Having directly the feedback of the use of these three features in production, the two remaining features specifications change :

• Modification of a user : it is not necessary anymore to modify a user as it is easy to delete a user and to create a new one
• Search a user by login by first name : is is finally better to search the user by his first name.

And the developer modifies the code

As it is not necessary anymore to modify a user, the method "updateUser" has been created for nothing. And as the user search must be done by the first name rather than by the user login : the searchUserByLogin method must be changed.

The developer still thinks that the "updateUser" method could be useful in the future, so he doesn't delete it. Moreover, he keeps the "searchUserByLogin" method and create a new method "searchUserByFirstName".

So finally in the DAO layer, we have six methods but only four methods are used :

public interface UserDao {
  public void createUser(User user);
  
  public void updateUser(User user); // useless
  
  public void deleteUser(User user);
  
  public User searchUserByLogin(String login); // useless
  
  public User searchUserByFirstName(String firstName);
  
  public List getUsers();
}

The main mistake the developer did is to add unnecessary code in his program because he was thinking it could be useful in the future. The second mistake he did is to don't clean his code after the specifications change.

Maybe you don't realize why these two methods can be a problem in your code. So try to imagine several developers doing this mistake every day : it will be quickly the mess in your code.

You Ain't Gonna Need It!

Based on the fact that we cannot know or guess the future needs of a software, the idea of YAGNI is to never add code or feature which is not immediately necessary.

More than avoid to have useless methods in the code, the main advantages of the YAGNI principle are :

• Your code is smaller, so more simple 
• You don't have to maintain unnecessary code so you win time when you refactore your code 
• You don't have pieces of code which have never been tested in production and you don't risk to use it thinking it is working