How to do a code review • Hacker News Board

Re: How to do a code review

#131

Post by proverbialbunny » Fri, Sep 06, 2019, 2:17 AM UTC

mieseratte wrote: post #25

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…

Why is it that people feel so discouraged by loads of review, especially early on? I always had a good bit of imposter syndrome early on, but never considered quitting. I always assumed that you have a lot to learn, that it’s expected you’re going to suck at some level.

>Why is it that people feel so discouraged by loads of review, especially early on?

Many people often think their skills and actions are them, instead of things they've acquired.

The question you're asking is oddly similar to asking why people get stressed and mentally suffer at all. It's a deep and complex subject. Taking is personally only scratches the surface.

Eg, I know someone who switched her career over it. I asked her why she left and she told me they kept criticizing her, and she took it as if they were attacking her and trying to get her to quit.

Re: How to do a code review

#132

Post by abacadaba » Fri, Sep 06, 2019, 2:18 AM UTC

tayo42 wrote: post #103

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…

If your making mistakes like over engineering are you actually a good developer then if that is part of what bad code is. Maybe you fall into the expert begginer at that point

I'll take a noob who doesn't understand basic syntax all day every day vs. the dev who finds a way to make everything complex. At least in the former the damage is limited.

Re: How to do a code review

#133

Post by flowerlad » Fri, Sep 06, 2019, 2:18 AM UTC

jonahx wrote: post #123

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…

Preface: I was also very glad to see this called out specifically, and think it's a great rule. That said... > Note how Google does NOT say "make sure the code is properly architected". is not accurate. The very first paragraph on the same page is: > Design > The most important thing to cover in a review is the overall design of the CL. Do the interactions of various pieces of code in the CL make sense? Does this cha…

But that sure sounds like they care about proper architecture.

Of course they care about architecture—I don’t think anyone implied otherwise. But at good companies like Google good architecture is a given. Top developers often fall into the pit of over-engineering, and almost never under-architect. So at top companies code reviewers have to be vigilant about over-engineering and rarely have to worry about under-architecting.

Re: How to do a code review

#134

Post by bt848 » Fri, Sep 06, 2019, 2:19 AM UTC

molodec wrote: post #125

polishTar wrote: post #69

Earlier quoted context omitted.

> Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. That's way too strict. If I want to suggest moving a statement that's inside a loop to the outside, I don't want to waste my time writing microbenchmarks showing that it improves performance. If the suggestion is complicated or non-obvious, and it's in a reall…

I agree that "must" is too strict. If I replace linear search with binary search, or hashmap/hashset lookup I don't need to write a benchmark to prove it improves performance. There is math, logic, Big O analysis that allows to reason about performance and speed without microbenchmarks.

If you did that to code in Google search you’d be required to run a special load test to prove you didn’t screw it up. Nobody should assume either of the things you just implied were obvious. In fact linear search is guaranteed to beat binary search for short vectors. O-complexity analysis is good for undergrads but the ONLY aspect of software performance that matters any more is cache behavior.

Re: How to do a code review

#135

Post by [deleted] » Fri, Sep 06, 2019, 2:24 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…

[deleted]

Re: How to do a code review

#136

Post by sanderjd » Fri, Sep 06, 2019, 2:25 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…

In that case you should have an artifact to point to that describes the plan at a higher level. So you send a diff with a newly expanded role for some character, the summary of the diff says, "Refactor chapter 1 to prominently feature Character", then the detailed description says, "Character will have an arc that culminates in Conflict in Act 2 and Resolution in Act 3. See {link} for more details on my current thoughts on where Character is heading." Then this new prose in chapter 1 also has to be good when taken on its own, no spelling or grammar errors and a generally nice sound to it. Then I would consider such a diff eminently mergeable.

That is to say, a change doesn't have to be perfect on its own, but the direction should be clear, and it should be locally well constructed.

Re: How to do a code review

#137

Post by CalChris » Fri, Sep 06, 2019, 2:25 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…

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

Re: How to do a code review

#138

Post by Rapzid » Fri, Sep 06, 2019, 2:25 AM UTC

bt848 wrote: post #53

I'd add two things, from a decade of experience at Google: Review code in this order: protocol buffers, unit tests, headers, implementation. It's common for a new employee to be an expert on C++ or Java or whatever languages but it's very uncommon to meet anyone who knows how to define a decent protocol message. The tests should give the reviewer a very clear idea of what's happening in the code (and this should be c…

Those are an oddly specific and narrowly focused set of recommendations to add to a set of guidelines that are largely very general and apply to a broad range of circumstances..

Re: How to do a code review

#139

Post by pjm331 » Fri, Sep 06, 2019, 2:26 AM UTC

derekp7 wrote: post #116

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…

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 think of it this way: How can I build this so that it only solves today’s problems but doesn’t make it overly difficult to solve tomorrow’s problems?

Loose coupling, dependency injection, composition over inheritance, and similar techniques tend to be good answers to this question in my experience.

In contrast, over engineering attempts to solve tomorrow’s problems before they arrive and, if they arrive differently than predicted, makes them harder to solve, because when you have to change something it’s less clear which parts of the design were necessary to solve the original problem and which were only necessary for the future problem that was incorrectly predicted. Often times you might end up having to rethink the entire architecture rather than having the relatively simple problem of adjusting things to meet new requirements.

Re: How to do a code review

#140

Post by siquick » Fri, Sep 06, 2019, 2:28 AM UTC

hckr1292 wrote: post #33

siquick wrote: post #22

Earlier quoted context omitted.

> I've seen cases where people got hundreds of comments (many of them minor, nitpicky) from more experienced developers and were discouraged by the sheer number of them. That most new developers naturally suffer from imposter syndrome is not helped at all by 100% critical code reviews. This, combined with a large portion of developers lacking social empathy, poor communication skills, and (unfortunately) a desire to…

Social empathy is a really important skill, especially when delivering feedback. However, the best way to improve as a junior developer is to welcome feedback, not panic. I've seen junior developers welcome feedback or run from it, and I think that has as much to do with the reviewer as the author of the code. On the flip side, I've learned almost as much from reviewing more senior engineer's code and just asking dum…

I'm sure we've all worked in places where a senior would not like it if a junior reviewed their code, regardless of the juniors intent. But yes, always a tonne to learn.

IMO the first few code reviews should be done face to face so some rapport can be built. Receiving feedback from a 'human' is far easier to process than a faceless Github profile picture.