How to do a code review • Hacker News Board

Re: How to do a code review

#141

Post by codeGreene » Fri, Sep 06, 2019, 2:29 AM UTC

CalChris wrote: post #137

petilon wrote: post #99

Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…

This is for a code review which would be well after architecture and be the entirely wrong place to systematically question architecture.

To be honest, after reading this document, I believe it contain MUCH more information than just how to conduct a code review.

Re: How to do a code review

#142

Post by jchanimal » Fri, Sep 06, 2019, 2:32 AM UTC

flowerlad wrote: post #129

derekp7 wrote: post #116

Earlier quoted context omitted.

It sounds like walking a tight line between over engineering, and falling into technical debt. If you design code specifically solves the immediate need, that may need to be thrown away or extensively worked on / around when future needs come up. On the other hand, you can write code that solves future needs that never appear, and still fail to solve the actual needs that end up appearing. For me, I would rather put…

I can tell you what I do: I allow the total cost to increase by no more than 10% for future proofing. It is all about controlling cost. It is not justifiable to spend more than 10% of the time for future proofing because you have no idea what the future is going to be. Of course if you do have some idea about requirements coming in the near future then it may be justifiable to spend more.

It’s more than a flat cost estimate. It’s more about risk mitigation. Trying to add multi-tenancy to a system that wasn’t designed for it, can mean a major rewrite. Building the system from the beginning with the idea that there could be multiple tenants may cost 10% more, but the opportunity cost of not being able to address the multi-tenant market could be much bigger.

Re: How to do a code review

#143

Post by sailfast » Fri, Sep 06, 2019, 2:32 AM UTC

ereyes01 wrote: post #47

aviraldg wrote: post #13

This is great advice and isn't followed often enough, especially when reviewing code written by people new to an organization/team: > If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, i…

I once worked on a team that specialized in very long littanies of code review comments... but they were able to bake this into their culture in fundamental ways such that it ended up being one of the most positive experiences in my software engineering career. The basics of how they accomplished this was: - The obvious- no personal / destructive attacks or insults, no cussing, no comments on any person's abilities.…

Sure, but “Hey I really appreciated how you did this thing here because it’s tidy / does thing X really well / takes into account future whatever” never hurts to throw in either!

Re: How to do a code review

#144

Post by ScottBurson » Fri, Sep 06, 2019, 2:33 AM UTC

d_burfoot wrote: post #59

Am I the only software engineer in the world who hates diff-based code review? Imagine trying to evaluate the quality of a novel by examining diffs to the manuscript. Reviewing every diff means that every single change needs to yield a good novel. But that's an absurd constraint on the creative process - what if you want to introduce a new important character? You check in a new version of the first chapter where the…

I wouldn't say I "hate" diff-based code review; it can serve a useful purpose; but I do find that unless I'm very familiar with the code being changed, I don't have time to fully understand the impact of the proposed changes. It isn't until later, when I need to modify or extend a piece of code, that I do that level of analysis — and of course I often find problems then that no one else had noticed (often in my own code too).

In my last job we didn't have a review requirement for commits; we mostly relied on this form of catching problems after the fact (and, of course, an extensive test suite). For experienced developers I thought it worked fine; it was only some of the junior people of whom I thought we should probably be reviewing their code more regularly.

In my current job we do have such a requirement, and I have mixed feelings about it. There are still plenty of times I don't manage to catch mistakes before they get merged in, and the delays getting someone to review my PRs can be frustrating. The system clearly has some value, but it also has costs. I think I would allow most developers, after an initial probation period, to commit without approval, keeping in mind that someone who shows a pattern of committing bad code may lose this privilege.

Re: How to do a code review

#145

Post by sailfast » Fri, Sep 06, 2019, 2:33 AM UTC

civicsquid wrote: post #82

aviraldg wrote: post #13

This is great advice and isn't followed often enough, especially when reviewing code written by people new to an organization/team: > If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, i…

Something I've started doing that I picked up was prefixing my nitpicky comments with "Nit: ..." so that it's clear that certain comments are just minor suggestions, not that anything is necessarily wrong. I'm usually okay with preemptively accepting code with only nit comments too just to signal that those comments are not too big of an issue (if at all).

The guide goes into this as well with the “LGTM/Comments” approval flow. It is something we’ve done organically but nice to see written up as well.

Re: How to do a code review

#146

Post by vonseel » Fri, Sep 06, 2019, 2:34 AM UTC

pitaj wrote: post #114

vonseel wrote: post #112

Earlier quoted context omitted.

> hundreds of comments (many of them minor, nitpicky) This is one reason I liked Phabricator’s review system, which allowed drafting comments on an entire PR before submitting the comments. This allows you to be as nit-picky as you want when reading the PR, and then delete or modify any of them at the end. Instant submission of line-level comments, on the other hand... I think reviewers should have the awareness to u…

GitHub features the same draft system you described.

I thought theirs was immediate, but I was referring mostly to Bitbucket Server's system from previous experience with that.

Re: How to do a code review

#147

Post by royalghost » Fri, Sep 06, 2019, 2:40 AM UTC

When a development team is following XP Principles, mostly two developers are doing Pair Programming with Test Driven Development (TDD) and rotating pairs every week, how much value will it add with code review ? We recently had a discussion where some of us think it is still good to skim through the change request by a second pair while other's think it's just a waste of time.

I am curious to know if code review is encouraged by a second pair in following XP practices ?

Re: How to do a code review

#148

Post by underwater » Fri, Sep 06, 2019, 2:40 AM UTC

petilon wrote: post #99

Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…

I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code.

Re: How to do a code review

#149

Post by chairleader » Fri, Sep 06, 2019, 2:50 AM UTC

I appreciate this:

> In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.

Reviewers are human too, and can occasionally get lost in the weeds nitpicking a PR/CL.

Re: How to do a code review

#150

Post by shakezula » Fri, Sep 06, 2019, 2:50 AM UTC

> if a reviewer makes it very difficult for any change to go in, then developers are disincentivized to make improvements in the future.

This is a problem I see far too often but it’s rarely talked about. Too often, engineers misinterpret “quality” code for “their” code. Code review turns from “what should we be doing here?” Into “what would this reviewing engineer name this one unimportant variable here?”

There needs to be a happy medium between velocity and quality, and increasing velocity doesn’t necessarily mean decreasing quality.