How to do a code review • Hacker News Board

Re: How to do a code review

#271

Post by d33 » Fri, Sep 06, 2019, 9:02 AM UTC

siempreb wrote: post #261

What is good design, what is good naming, over-engineering, etc, etc.. How on earth can a group of people agree on all those points? Look around here on HN, all those discussions.. Do we have consensus? No, and that is OK, but it's not in a team by code review. There is only one way a group of people can have healty code reviews, that is when they like each other so much, or enough to accept what they not agree on. B…

> what is good naming

It's mostly a "I know it when I see it" kind of thing. We know most of the time what bad naming looks like - it creates confusion, tells you nothing about the variable ("i", "n", "val", etc). I guess this is not a science (yet?) and we're currently at the stage of defining anti-patterns, out of which maybe we'll figure out some actual desired patterns.

Re: How to do a code review

#272

Post by LandR » Fri, Sep 06, 2019, 9:10 AM UTC

mytailorisrich wrote: post #229

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…

Overall architecture should have been discussed and agreed before writing any code, not at code review.

In an ideal world, yes. In practice this almost never happens. Architecture evolves as the code is written.

I don't think this is a good way to do things, but it's mostly what happens IME.

Re: How to do a code review

#273

Post by meitham » Fri, Sep 06, 2019, 9:10 AM UTC

Traubenfuchs wrote: post #245

I have never not seen code reviews end up being a massive social/cultural pain point for most people involved. Most people hate getting their code reviewed in depth. In situations where there isn't just one lead dev who is responsible for reviews, this leads to "Merge Request symbiosis", where two people uncritically approve each others requests so they don't have to deal with the third.

I agree, my role of thumb is to only review code if I was asked by the author to do so, and never offer to review someone's else code.

Code is a bit like music, there is very little right or wrong, but a lot of flavours and opinions.

Re: How to do a code review

#274

Post by LandR » Fri, Sep 06, 2019, 9:13 AM UTC

rajangdavis wrote: post #262

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…

100's of comments should raise a red flag. I think there are 1 of 3 scenarios at play: 1. The commit/change is too large. 2. The reviewer is nit-picky. 3. The person that wrote the code made a lot of mistakes. I really feel like if a reviewer is making 100 comments on a change, they're doing something wrong even if there are a lot of mistakes. That reviewer should really reach out to the person that wrote the code an…

As someone who did a code review for a colleague and left over 100 comments... I would say that leaving those comments is handy as it's a single place I can check to see what comments I left and the state of the fix, or if it's not fixed yet.

If I just ended up talking to the guy, I'd have no record and he would have no record (unless he took notes) of all the places he has to fix.

The reason for 100+ comments was a junior developers code and far too large a changeset.

Re: How to do a code review

#275

Post by cjfd » Fri, Sep 06, 2019, 9:16 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…

> Over-engineering is a trap that very good developers fall into all too often.

I would not call them 'very good developers' in that case.

Re: How to do a code review

#276

Post by Deradon » Fri, Sep 06, 2019, 9:17 AM UTC

esolyt wrote: post #220

Helpful and interesting. However, you can tell this was written by an engineer. The acronym CL is used about a hundred times. There is not a single page where the first usage of the acronym explains the full term.

It is explained at the top-level document: https://google.github.io/eng-practices/#terminology

Re: How to do a code review

#277

Post by antpls » Fri, Sep 06, 2019, 9:32 AM UTC

shusson wrote: post #270

antpls wrote: post #265

How at this point code review not entirely automated? There are numerous fuzzy tools, memory leak detectors, linters and standards. Code is data, and it can be analyzed as much as one wants with programs. Sonarqube for example can find bad patterns, code smells, etc, there is no limit to what can be added to such tools. As long as you don't want to prove that the code terminates, you can probably write an automated c…

> How at this point code review not entirely automated? There is no automated way to check intention. I can write perfectly correct code which also drops the database.

In this case, wouldn't you use some kind of permission/security groups/roles at the db level or firewalls that would block such intents?

Wouldn't it be possible to add an automatic linter pass that does "grep DROP" on the source code? Even unusually things such as code obfuscation can be automatically detected to some extent.

An automated way sounds more fair, robust and scalable. To me, there is something in human code review that is not engineering, it's all about judging people (unless the code review is fully anonymous for both side)

Re: How to do a code review

#278

Post by lostmyoldone » Fri, Sep 06, 2019, 9:44 AM UTC

jwl wrote: post #233

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…

Yes, I don't think I have ever seen a premature optimisation actually ending up being beneficial when the time came to add new features to the project.

Premature optimization rarely helps, but well though out flexibility/decoupling in core system components has had a significant positive effect on velocity down the line, and lack of the latter has all been shown to be disastrous.

I do believe though that's really hard to discuss effectively as there seems to be no good, and common definition on what over engineering actually is, except in retrospect.

I've seen teams where they were so "good" at avoiding over engineering and "architecture astronauts" that thousand line functions with a Byzantine labyrinth of conditional was preferred to even the most basic of design.

With that said, what would you consider over engineering of the kind that never works, and in what kind of systems?

Re: How to do a code review

#279

Post by chrisseaton » Fri, Sep 06, 2019, 9:56 AM UTC

kortilla wrote: post #222

flowerlad wrote: post #133

Earlier quoted context omitted.

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-archi…

>But at good companies like Google good architecture is a given. Absolutely not. Google’s interview process and inflow of fresh graduates does not bode well for good architecture. Having spent time at G and FB, I can certainly tell you that employees at both are no better at architecting code in a sane way than SWEs at other companies. Code architecture requires experience. Google does not.

I think the hiring bar at places like Google and FB is astronomically higher than almost all other places - they definitely have generally higher skilled people there on average.

Re: How to do a code review

#280

Post by jsnell » Fri, Sep 06, 2019, 9:57 AM UTC

BeetleB wrote: post #158

molodec wrote: post #125

Earlier quoted context omitted.

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.

bt848 already said it, but you picked a really poor example. In many real world scenarios, a linear search beats a binary search due to a lack of overhead. Along those lines, in C++, a vector very often performs better than a theoretically better data structure. I learned C++ fairly well from a very experienced guy in the company, and he said that you should always benchmark against a vector. I suspect if I was in hi…

So now the requirement is not just to run a micro benchmark, but to run one with inputs that approximate the distribution of production inputs, along all possible axes. For many sorts of projects, this is totally unreasonable. It's easy to figure out how the code performs with a given input, it is much harder to figure out what the inputs really are like.

In this particular case I'd expect the change to be motivated by profiling of the real production instances. And that makes it pretty obvious how the change should be evaluated. "We're spending more time than is reasonable in linear scans, so the worst case inputs must be worse than expected. Switch to a data structure more suited to large inputs, and see if CPU use improves in the next rollout."