How to do a code review • Hacker News Board

Re: How to do a code review

#281

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

Defer all decisions until you have to make them.

Leave the code in a state where it's understandable.

ie don't over specify - not just simple things like interfaces, but also things like premature decomposition, that's exposed at the higher level for configurability.

While on the face of it, having everything as small interacting functions means to change any function is quite easy, changing overall behaviour might be very hard - hard to understand all the moving parts, and even worse, if the code split is wrong for future needs, very hard to change.

Re: How to do a code review

#282

Post by lostmyoldone » Fri, Sep 06, 2019, 10:32 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.

At work, and especially when you are new you are at the mercy of others in several ways to an extent that usually eclipses almost anything we expose ourselves to in our lives.

Reviewing can thus be experienced as opening up yourself to critique by essentially anonymous "kingmakers", which at any time can make you look like a clown, if they want to. This experience might not be rational, but I believe it's still extremely common, as an experience.

It's made worse by the fact, that if the review culture is anything else but great, it's not at all unlikely to try and take advantage of that - in one of many ways - in order to feel a bit better about themselves.

At our core most of us are social creatures, and thus most of us easily pick up on that danger/risk, including most that are otherwise not socially adept, as they have usually suffered bullying of some kind and have learned to avoid those exposed situations.

Re: How to do a code review

#283

Post by hagy » Fri, Sep 06, 2019, 10:44 AM UTC

What about the importance of handling high-level architecture and design before writing code? This can be done in design docs, discussion on tickets, slack discussions or in person with a whiteboard. I think it's more productive to handle these discussions and reviews before writing any code.

What do other people think?

Re: How to do a code review

#284

Post by mianos » Fri, Sep 06, 2019, 10:58 AM UTC

chrisseaton wrote: post #279

kortilla wrote: post #222

Earlier quoted context omitted.

>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.

The process does not encourage experience but mainly the ability to remember recent academic programming puzzles. The first comment is correct even though they do have many existing experienced developers that would be in a position to provide architectural advice.

Re: How to do a code review

#285

Post by ChrisCinelli » Fri, Sep 06, 2019, 11:00 AM UTC

A great guide. Maybe it is out of scope but I would have liked to see what they do BEFORE they start writing code.

The reason is because by the time to get to a code review may be too late. The comment that you should give is "this is not how it should be implemented. You need to rewrite it from scratch." But most of the time you do not.

Re: How to do a code review

#286

Post by rajangdavis » Fri, Sep 06, 2019, 11:04 AM UTC

LandR wrote: post #274

rajangdavis wrote: post #262

Earlier quoted context omitted.

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…

I feel as though leaving 100 comments only addresses the code but not the coder.

Telling someone that the changes they made are too much for a commit would probably go a lot farther.

Identifying common architectural problems would also reduce what you need to connunicate. Maybe they made a lot of mistakes but I would find it difficult to believe there were 100 unique mistakes.

Re: How to do a code review

#287

Post by ElonMuskrat » Fri, Sep 06, 2019, 11:05 AM UTC

Excellent guide. It would be fantastic for Google to adopt these practices.

Re: How to do a code review

#288

Post by ChrisCinelli » Fri, Sep 06, 2019, 11:09 AM UTC

ChrisCinelli wrote: post #285

A great guide. Maybe it is out of scope but I would have liked to see what they do BEFORE they start writing code. The reason is because by the time to get to a code review may be too late. The comment that you should give is "this is not how it should be implemented. You need to rewrite it from scratch." But most of the time you do not.

In my team I like to have a "technical kickoff meeting".

But before that, the person that will implement the story will goes through a checklist that I started and we improved during retros. The engineer sits down with another engineer and they go over again together. If they did their homework it is usually pretty quick.

Sample for web dev:

-Does this feature heavily relay on or modify code that someone else wrote? Should we talk with her? Should we HAVE THEM in this MEETING? In particular for BE work who should we talk?

- What names are we going to use for Components, Pages, Folders, etc?

- Is the visual design/flow clear and complete? - Do we have every assets from design? Do we need to ask for more? - Do we have dependencies on other work being done now on a branch or in the near future ? - Do we have overlap with other pieces of code (possible conflicts)?

- Will we reuse some other components or any other relevant utility functions?

- What code can be reused?

- Do we want to make some of the code for this feature reusable?

- (Redux) What reducers will we use (if any)? In particular do we need new ones?

- What are the most important tests to write ?

- Are we going to phase the feature?

- Do we have to compromise and create some tech debt? If yes, what, why and when do we plan to pay it?

I found that this meeting makes most of the code reviews fast as well.

Re: How to do a code review

#289

Post by badpun » Fri, Sep 06, 2019, 11:11 AM UTC

colek42 wrote: post #172

abacadaba wrote: post #132

Earlier quoted context omitted.

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.

Good dev's understand how to simplify complex problems by breaking it into smaller components. Bad devd add complexity to already complex problems.

Breaking down complex problem does not simplify it, you merely move the complexity to the graph of dependencies between smaller components.

Re: How to do a code review

#290

Post by krona » Fri, Sep 06, 2019, 11:12 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 only works at Google because most of the code is rewritten every few years [1]. The dynamics between tech debt and over-engineering in such a setting are not really representative of the rest of the industry.

1: https://arxiv.org/pdf/1702.01715.pdf