Open Stack

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