CodeReviewGuidelines - Revision history

Smaffulli: Undo revision 71468 by Smaffulli (talk)

2015-01-09T18:12:28Z

Undo revision 71468 by Smaffulli (talk)

Smaffulli: /* Further reading */

2015-01-09T18:03:01Z

‎Further reading

Smaffulli: /* Further reading */

2015-01-09T18:02:38Z

‎Further reading

Shari at 22:00, 9 October 2014

2014-10-09T22:00:19Z

Pavlo Shchelokovskyy: Added a link to maillist thread with discussion about bad review patterns

2014-09-05T12:06:11Z

Added a link to maillist thread with discussion about bad review patterns

Miranda: /* Not sure if it works */ add missing word "author" in line

2013-12-12T08:52:41Z

‎Not sure if it works: add missing word "author" in line

Matthew Mosesohn: /* Leave constructive comments */

2013-12-11T16:15:09Z

‎Leave constructive comments

Matthew Mosesohn: /* Don't repeat yourself */

2013-12-11T16:12:03Z

‎Don't repeat yourself

Radomir Dopieralski: Some guidelines for code reviewers, as discussed on the mailing list.

2013-11-07T10:59:13Z

Some guidelines for code reviewers, as discussed on the mailing list.

New page

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.

== Show examples ==

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 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.

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.

== Old bugs ==

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 ==

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.

← Older revision		Revision as of 18:12, 9 January 2015
Line 38:		Line 38:

	[[Category:Contribute]]		[[Category:Contribute]]
−	[[Category: ContribDocs]]	+	[[Category: ContribDocs

← Older revision		Revision as of 18:03, 9 January 2015
Line 38:		Line 38:

	[[Category:Contribute]]		[[Category:Contribute]]
−	[[Category: ContribDocs	+	[[Category: ContribDocs]]

← Older revision		Revision as of 18:02, 9 January 2015
Line 38:		Line 38:

	[[Category:Contribute]]		[[Category:Contribute]]
		+	[[Category: ContribDocs

← Older revision		Revision as of 22:00, 9 October 2014
Line 36:		Line 36:
	== Further reading ==		== Further reading ==
	[http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html This] maillist thread has very interesting discussion about bad review patterns,		[http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html This] maillist thread has very interesting discussion about bad review patterns,
		+
		+	[[Category:Contribute]]

← Older revision		Revision as of 12:06, 5 September 2014
Line 33:		Line 33:

	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.		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.
		+
		+	== Further reading ==
		+	[http://www.mail-archive.com/openstack-dev@lists.openstack.org/msg07831.html This] maillist thread has very interesting discussion about bad review patterns,

@@ Line 7: / Line 7: @@
 == 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 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.
+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 ==

← Older revision		Revision as of 16:15, 11 December 2013
Line 26:		Line 26:

	== Leave constructive comments ==		== 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.		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.

@@ Line 11: / Line 11: @@
 == 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.
+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.