Hello, I'm quite new in the OpenStack project, but I love it already. What is especially nifty is the automated review system -- I'm really impressed. I'm coming from a project in which we also did reviews of every change -- although it was mostly manual, and just one review was enough to merge -- and at some point in that project I noticed that it is very easy to give reviews that are unhelpful, frustrating and just get in the way of the actual work. I started paying attention to how I am reviewing code, and I managed to come up with several patterns that are bad. Once I know the patterns, it's easier to recognize when I'm doing something wrong and rethink the review. I would like to share the patterns that I noticed.
Not sure if that works... ========================= You see some suspicious piece of code, and you are not sure if it is correct or not. So you point it out in the review and -1 it, demanding that the author rechecks it and/or prove that it indeed works. It's your job as a reviewer to check such things, don't put all the effort on the author. They probably already checked that this code works, and possibly have even written tests for it. If you are not familiar with the technology enough to tell whether it's good or not, and have no means of testig it yourself, you shouldn't be reviewing it. If you do have the means to test it or can find the piece of documentation that says that it shouldn't be done -- do it as a part of the review. You ain't gonna need it. ======================== The code contains some parts that are potentially reusable later or in different areas, so you -1 it and tell the author to move them into reusable functions. The fact that you think something is reusable doesn't necessarily mean it is, and overly general code is harder to maintain. Let something repeat a couple of times just to be sure it actually is reusable. Once you find a repeating pattern, you can refactor the code to use a generalized function in its place -- in a separate change. There is an old bug here. ========================= While you review the submitted code, you notice something wrong in the code not immediately related to the change you are reviewing. You -1 the change and tell the author to fix that bug, or formatting issue, or typo, or whatever. That fix has nothing to do with the change you are reviewing. The correct thing to do is to make a mental note of it, and fix it in a separate change -- possibly even finding more instances of it or coming up with a much better fix after seeing more code. Guess what I mean. ================== You notice a pep8 violation, or pep257 violation, or awkward wording of a comment or docstring, or a clumsy piece of code. You -1 the change and just tell author to "fix it". It's not so easy to guess what you mean, and in case of a clumsy piece of code, not even that certain that better code can be used instead. So always provide an example of what you would rather want to see. So instead of pointing to indentation rules, just show properly indented code. Instead of talking about grammar or spelling, just type the corrected comment or docstring. Finally, instead of saying "use list comprehension here" or "don't use has_key", just type your proposal of how the code should look like. Then we have something concrete to talk about. Of course, you can also say why you think this is better, but an example is very important. If you are not sure how the improved code would look like, just let it go, chances are it would look even worse. Not a complete fix. =================== The code fixes some problems (for example, fixes formatting and enables some flake8 checks), but leaves some other, related problems still not fixed. You -1 it and demand that the author adds fixes for the related problem. Don't live your coding career through the authors. If their fix is good and correct and improves the code, let it in, even if you see more opportunities to improve it. You can add those additional fixes yourself in a separate change. Or, if you don't have time or skill to do that, report a bug about the remaining problems and someone else will do it, but don't hold the author hostage with your review because you think he didn't do enough. Leaving a mark. =============== You review a change and see that it is mostly fine, but you feel that since you did so much work reviewing it, you should at least find *something* wrong. So you find some nitpick and -1 the change just so that they know you reviewed it. This is quite obvious. Just don't do it. It's OK to spend an hour reviewing something, and then leaving no comments on it, because it's simply fine, or because we had to means to test someting (see the first pattern). Those are the things I'm careful about myself. I'm sure that not everyone of you will agree that all of those patterns are bad in all situations -- in fact, I'm pretty sure that some of them are sometimes warranted -- but they are always suspicious, and when I find myself falling into one of them, I always rethink what I'm doing. Maybe you have some more bad patterns that you would like to share? Reviewing code is a difficult skill and it's always good to improve it by using experience of others. -- Radomir Dopieralski _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev