How I review code

Without comments, sometimes it's really really difficult to navigate the code. I have been adding more comments than ever: don't assume every line is obvious, write a comment to explain what the next few lines really do.

    # base case: stop dividing when we find the largest square.
    if width == height:
        return width, height
    else:
        # otherwise, we know that we can break the land into several
        # pieces, some are already square-sized, but there must be
        # left over, which is the difference of the width and height,
        # and can be divided again.

        remain = abs(width - height)
        return largest_square_plot(remain, min([width, height]))

Some of my function/method has a pretty long docstring which may include explaining the rationale, and perhaps even some ascii diagrams. If you have trouble understand a piece of code after a few passes, that's a bad code. Also, use more newlines...

> I check every Github email I get; I make sure that I don’t get notified for everything that happens in the repo

Not sure about others, but I am tired of reading PR notifications in my mailbox. I don't know how kernel developers can live with this.

I have been thinking about just build a bot.

* receives PUSH from GitHub

* adds events to a queue

* notifies based on priority

* pings me once in a while to remind me that I have outstanding PRs to review (as reviewer and as author of the patch).

If someone needs me to review right away, he/she can reach out to me directly in chat.

Isn't this the point of code review? When I click accept on a code review, I am saying that I have looked over the change and believe that it is correct and okay. If I just arrive at the conclusion by saying "Joe wrote this, and I trust Joe" the there is no point in me reviewing it.

Personally I find that non-idiomatic code can be a bit distracting when I'm trying to evaluate the code for its overall approach and architecture.

when posting the review, you try to eliminate as much of the cruft as possible.

when reviewing code, sit down, you're actually going to read it for understanding.

your mutual goal is to improve code and product quality. if after a few passes something seems wrong, or unclear you raise it. now the two of you get to have a pleasant discussion about what the right thing to do is. maybe some rework. maybe just a comment about unhanded error conditions. maybe nothing.

if someone raises an issue that not important either way, just change it to save time. and don't raise something unless you think its important. so, if you both think its important, you can have a discussion and reach consensus.

i think thats the real problem, that somewhere along the way the consensus culture got lost. that was a really important thing.

It is not a matter of if the reviewer "suspects there is a bug", but rather a matter of if the reviewer is convinced that there is not a bug.

If the reviewer needs to have someone explain why every minor code change is correct, they are either lazy or incompetent (or the code is badly written). Further, it is generally a bad idea to go to the person who wrote the code to explain why it is correct, as you are then much more likely to make the same mistake the original author made. However, being "okay" often goes beyond correctness, and into business decisions. Often times, these business are not documented (or if they are documented, it is in some management document that is not linked to from the code), so in order to review it, the reviewer has to ask the author what the bussiness consideration that led to the change was.