Difference between revisions of "CodeReviewGuidelines"
m (→Further reading)
|Line 38:||Line 38:|
Revision as of 18:02, 9 January 2015
They say that reading code is twice as hard as writing it. On this page you can find some hints that can help you make better reviews. Please note, that those are not absolute rules -- the final decision is always yours.
The simplest way to talk about code is to just show the code. When you want the author to fix something, rewrite it in a different way, format the code differently, etc. -- it's best to just write in the comment how you want that code to look like. It's much faster than having the author guess what you meant in your descriptions, and also lets us learn much faster by seeing examples.
Not sure if it works
Sometimes you will see a piece of code that looks suspicious, or is so complex, that you are not sure if it behaves properly in all cases. In such cases you should take care to check it properly, spends time to analyze and test it, instead of simply asking the author to prove to you that it works. If you don't have the means to test it -- it requires specialized setup that you don't have, you don't have the time, etc. -- don't review it. When you do ask the author to explain it, though, it may be a good idea to ask the author to put the explanation in a comment in the code -- if it confused you, it may as well confuse others in the future. Also, adding a unit test that proves that it works properly in corner cases may be a good idea.
Don't repeat yourself
When there is an obvious repeating, copy-pasted pattern in the code, ask the author to abstract it away into a separate function. Best of all, show an example of how to do that in the comment. Focus on code consistency using existing functions and suggest options for simplification.
When the author has written some code that could be replaced by an existing utility function, point them to that function. Best, show an example of how it could be used there.
When the author submits some code that is similar to code from another part of the project, this is a great opportunity for you to do some refactoring. Follow up with your own patch that cleans up the code. If you don't have time for it, make sure to report a bug about it, so that you or someone else can do it later. Such easy to do bugs are a great way for new people to start contributing to your project, so it's nice to have some of them in the bug tracker.
Often, when reading the code of a patch, you will spot old bugs that are not immediately related to the patch in question -- they just happened to be within the diff context of the patch. When you see such a bug, either report it in the bug tracker, or follow up with a patch that fixes it. But don't tell the author to include a fix in their patch -- it's about something else, after all.
Don't inflate the scope
Sometimes a patch can be modified a little to solve a more general problem, and it's good to point that out, but don't block the patch for that. If it solves a problem and leaves the project better, it's good to include it, and the other problems can be solved in separate patches. If you see the opportunity, by all means make a followup patch or describe it in a bug or blueprint, but don't demand of the original author to implement your idea by blocking his patch.
Leave constructive comments
Not everyone in the community is a native English speaker, so make sure your remarks are meaningful and helpful for the patch author to change his code, but also polite and respectful.
The review is not really about the score. It's all about the comments. When you are reviewing code, always make sure that your comments are useful and helpful to the author of the patch. Try to avoid leaving comments just to show that you reviewed something if they don't really add anything meaningful.
Be there to discuss
Never disapprove a patch and then simply leave (especially when you know that you won't be available for longer time). Be sure to always come back to your unfavorable reviews to discuss with the author and reevaluate them. If you can't get an agreement, move the discussion to the mailing list and let other members of the community participate in it.
This maillist thread has very interesting discussion about bad review patterns,[[Category: ContribDocs