Code Review Benefits & Rules

Code Review

Aller directement à la fin des métadonnées

Aller au début des métadonnées

(It was only supposed to take one hour)

Code review is systematic examination (often known as peer review) of computer source code. It is intended to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and the developers’ skills. It's a thorough technical and logical line-by-line review of a code module. During a peer review, code is inspected to identify possible improvements and ensure that business requirements are met. If necessary, a meeting is held to discuss any issues that come out of the review process. Although some programmers complain that the reviews take too much time, the drawbacks are usually outweighed by the benefits, such as fewer bugs, less rework, more pride, and better team communication and cohesiveness. This guide will outline some of the positives of the process and how the actors should behave. This guide will give a suggestion of rules that can be used for code reviews.

Why code Review?

1. Team cohesiveness : Code review can be a real team building exercise, It increase and improves communication between team members. Though Code review must be given only keeping positives in mind, instead of criticizing other developer. Working together helps draw team members closer. It also provides a brief respite from the isolation that coding often brings 
2. Code review also increase knowledge sharing between team members and all the team remains in same page and ready to backup each other. This can be a huge plus for Manager, who needs to manage leaves and resources. Uniformity in understanding will help interchangeability of team members in case of non-availability of any one of them.
3. Code review promotes a better code quality, reduce testing time and less number of bugs. Code review helps to spot and fix defects early  & often in the process. 
4. Helps to maintain a level of consistency in design and implementation.
5. Code review would save half the cost of fixing the bugs. Plus they would have found x additional bugs.
6. Builds confidence of stakeholders about technical quality of the execution.
7. A different perspective. “Another set of eyes” adds objectivity.

Code reviews are often misused and painful for everyone, but they don't have to be. Some simple steps can convert torture into teaching and improve the long-term outlook for code quality in your organization. So let's start by defining the way a developer and reviewer should behave before setting the review rules.

Tips for the Developer:

1. The primary reviewer is the author i.e. YOU.
2. Create a checklist for yourself of the things that the code reviews tend to focus on. Run through your code with the checklist and fix whatever you find. Not only will you reduce the number of things that the team finds, you’ll reduce the time to complete the code review meeting—and everyone will be happy to spend less time in the review.
3. You are not your code. Remember that the entire point of a review is to find problems, and problems will be found. Don’t take it personally when one is uncovered.
4. Understand and accept that you will make mistakes. The point is to find them early, before they make it into production. Fortunately, except for the few of us developing rocket guidance software at JPL, mistakes are rarely fatal in our industry, so we can, and should, learn, laugh, and move on.
5. No matter how much “karate” you know, someone else will always know more. Such an individual can teach you some new moves if you ask. Seek and accept input from others, especially when you think it’s not needed.
6. Don’t rewrite code without consultation. There’s a fine line between “fixing code” and “rewriting code.” Know the difference, and pursue stylistic changes within the framework of a code review, not as a lone enforcer.
7. The only constant in the world is change. Be open to it and accept it with a smile. Look at each change to your requirements, platform, or tool as a new challenge, not as some serious inconvenience to be fought.
8. Fight for what you believe, but gracefully accept defeat. Understand that sometimes your ideas will be overruled. Even if you do turn out to be right, don’t take revenge or say, “I told you so” more than a few times at most, and don’t make your dearly departed idea a martyr or rallying cry.
9. Don’t be “the guy in the room.” Don’t be the guy coding in the dark office emerging only to buy cola. The guy in the room is out of touch, out of sight, and out of control and has no place in an open, collaborative environment.
10. Please note that Review meetings are NOT problem solving meetings.
11. Help to maintain the coding standards. Offer to add to the coding standards for things discussed that aren’t in the coding standards. One of the challenges that a developer has in an organization with combative code review practices is that they frequently don’t know where the next problem will come from. If you document each issue into the coding standards, you can check for it with your checklist the next time you come up for code reviews. It also will help cement the concept into your mind so that you’re less likely to miss opportunities to use the feedback.

 Tips for the Reviewer

1. Critique code instead of people – be kind to the coder, not to the code. As much as possible, make all of your comments positive and oriented to improving the code. Relate comments to local standards, program specs, increased performance, etc.
2. Treat people who know less than you with respect, deference, and patience. Nontechnical people who deal with developers on a regular basis almost universally hold the opinion that we are prima donnas at best and crybabies at worst. Don’t reinforce this stereotype with anger and impatience.
3. The only true authority stems from knowledge, not from position. Knowledge engenders authority, and authority engenders respect – so if you want respect in an egoless environment, cultivate knowledge.
4. Please note that Review meetings are NOT problem solving meetings.
5. Ask questions rather than make statements. A statement is accusatory. “You didn’t follow the standard here” is an attack—whether intentional or not. The question, “What was the reasoning behind the approached you used?” is seeking more information. Obviously, that question can’t be said with a sarcastic or condescending tone; but, done correctly, it can often open the developer up to stating their thinking and then asking if there was a better way.
6. Avoid the “Why” questions. Although extremely difficult at times, avoiding the”Why” questions can substantially improve the mood. Just as a statement is accusatory—so is a why question. Most “Why” questions can be reworded to a question that doesn’t include the word “Why” and the results can be dramatic. For example, “Why didn’t you follow the standards here…” versus “What was the reasoning behind the deviation from the standards here…”
7. Remember to praise. The purposes of code reviews are not focused at telling developers how they can improve, and not necessarily that they did a good job. Human nature is such that we want and need to be acknowledged for our successes, not just shown our faults. Because development is necessarily a creative work that developers pour their soul into, it often can be close to their hearts. This makes the need for praise even more critical.
8. Make sure you have good coding standards to reference. Code reviews find their foundation in the coding standards of the organization. Coding standards are supposed to be the shared agreement that the developers have with one another to produce quality, maintainable code. If you’re discussing an item that isn’t in your coding standards, you have some work to do to get the item in the coding standards. You should regularly ask yourself whether the item being discussed is in your coding standards.
9. Remember that there is often more than one way to approach a solution. Although the developer might have coded something differently from how you would have, it isn’t necessarily wrong. The goal is quality, maintainable code. If it meets those goals and follows the coding standards, that’s all you can ask for.
10. You shouldn’t rush through a code review - but also, you need to do it promptly. Your coworkers are waiting for you.
11. Review fewer than 200-400 lines of code at a time.
12. The best reviews include thinking questions. Questions such as "Would it be better to implement a provider pattern here?" don't necessarily need a response. Make it clear to the developer that some comments are simply to provoke thinking. This allows you to identify areas where you want to make sure the developer is considering alternatives without raising them to the point of being an issue with the code. Getting developers to think about the code they are writing doesn't improve the quality of the code they've already written, but it does have a long-term positive impact on code quality.

Code review Rules (To be discussed and improved) :

The GOLDEN rules that are mandatory to be checked/enforced during code review :

1. Verify code compiles and includes javadoc where appropriate 
2. Verify code meets functional requirement: first and foremost does code meets all requirements which it should met, point out if anything has been left out.
   
3. Verify is there any Side effect of this change
4. Readability and maintenance:  does code is readable? Verify that whether code is configurable or not, look for any hard coding, find out what is going to be changed in near future etc.
5. Verify Corner cases and workarounds (framework limitations) are well documented 
6. Verify that comments are comprehensible and add something to the maintainability of the code(Comments are neither too numerous nor verbose)
7. Intention-Revealing Names Meaningful Names (Naming of variables and methodes and classes is very important so they should be selected carefully)
8. Functions should not take too many input parameters (1 parameter is enougth, 2 is acceptable, avoid more then 3)
9. Verify The code complies to coding standards
10. Verify Access modifiers (default, public, private, protected) are correct 
11. Verify methods and classes focus on doing one thing. and are short as possible.
12. Verify return types (Return empty arrays or collections, not nulls )
13. Verify that hashCode is overiden when you override equals (Otherwise a violation of the general contract for Object.hashCode will occur, which can have unexpected repercussions when your class is in conjunction with all hash-based collections.)
14. Verify that the code is using the same formatter checkstyle agreed by the team.
15. Never write specific logic on the Facades / Controllers / DTO / Daos
16. Respect the layers (services should not call services from other modules -> avoid module dependencies)
17. Verify that the service layer does not return entities but only DTOs
18. Verify transactional use cases (reasonly , propagation, isolation)
19. Verify the use of an interface for every spring component, Refer to objects by their interfaces
20. Promote “stateless” functionality
21. Check the validation exists and is correct for input objects. Ask the question : does code handles bad input and exception? Throw exceptions appropriate to the abstraction. Do not use generic messages in exceptions.
    
22. Reuse of existing code : Check if we are re-using existing functionality, avoid duplications , refactor if necessary
23. Avoid excessive synchronization, Keep Synchronized Sections Small
24. Verify Performance was considered
25. Verify NPEs and AIOBs are handled
    
26.  Promote usage of Java 8 language whenever possible
27. Verify needless and commented code is removed  (IDE helps)
28. Verify inexistence of unused imports and  IDE warnings  (IDE helps)
    
29. Verify Unit Test/ integration tests exists for the task and verify Failure/Success (mvn test)
30. Ensure that the unit tests are written properly. Don’t write unit tests for the sake of writing one. Describe the scenario you are testing in a comment if it's not obvious
31. Check how the test verifies the existence of the bug (run on existing code) 
    
32. Examine edge cases are covered by tests

References:

1. Clean Code Book
2. http://blog.codinghorror.com/the-ten-commandments-of-egoless-programming/
3. http://www.developer.com/tech/article.php/3579756/Effective-Code-Reviews-Without-the-Pain.htm