Live data from Hacker News

Code Review Handbook

sledgeworx.io

11–20 of 51 posts

Re: Code Review Handbook

#11
post #6

The reality is that most of your comments will be reminding people to follow the style guide or to not organize their code like monkeys with type writers. If you're leaving comments about style on PRs then your tooling isn't doing a good enough job. You should have a linter and a formatter configured, everyone should be sharing the same config, and they should always be run on a prepush hook (or earlier... run them o…

Style is much more than formatting. It's best practices around naming, when and how to add comments, and how to write clear and maintainable code.

Re: Code Review Handbook

#12
post #7

It's interesting that the author puts "downloading the code" in the exhaustive section. Is it really that much work to run git checkout? I routinely checkout code in PRs if I'm not familiar with that part of the codebase. I can navigate the codebase much quicker in my editor than in GitHub. Although GitHub is actually getting there with the semantic analysis of code and finding references and stuff, sometimes I just…

On bad projects (which happens often now that node_modules directories are often gigabytes big), IntelliJ IDEA is super-long to index, which is required before being able to Cmd+Click on functions. And we have M1.

But this is an example where usability impacts the author’s judgement. For me, UX testing is part of the initial step of code reviews, no technical review is necessary if we end up deciding to implement the story another way.

Re: Code Review Handbook

#13
It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes.

Also, I'd like to push back on this:

> Most developers have a lot of features to add and bugs to write, time spent reviewing other people’s work does not add a lot to your bottom line.

I disagree; it adds a lot to your bottom line. In fact, reading and reviewing code is one of the main tasks of a developer, perhaps more important than writing code.

Reviewing code gives you an opportunity to get familiar with other parts of the code base, understand why a change or fix is done, and collaborate with others to make an overall better improvement to the product.

It gets frustrating when egos get involved, and people take criticism too personally, or point out improvements for the sake of demonstrating their knowledge. In best working environments, code is judged without a bias or relation to who produced it, but as an inert output of the collective team. I would actually like to see a feature in code review tools to submit merge requests anonymously, just so that the author isn't a prevalent factor while reviewing. :)

I usually dedicate a few hours, maybe even half a day, to reviews. Definitely don't look at it as a chore to put off, but as a crucial part of your responsibilities as a developer.

Re: Code Review Handbook

#14
post #11
post #6

The reality is that most of your comments will be reminding people to follow the style guide or to not organize their code like monkeys with type writers. If you're leaving comments about style on PRs then your tooling isn't doing a good enough job. You should have a linter and a formatter configured, everyone should be sharing the same config, and they should always be run on a prepush hook (or earlier... run them o…

Style is much more than formatting. It's best practices around naming, when and how to add comments, and how to write clear and maintainable code.

I don't think I'd call any of those things "style". Those things are the code itself, not the style it's written in. They're definitely things that PR reviewers should be looking at.

Re: Code Review Handbook

#15
post #13

It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes. Also, I'd like to push back on this: > Most developers have a lot of features to add and bugs to write, time spent reviewing other people’s work does not add a lot to your bottom line. I disagree; it adds a lot to your bottom line. In fact, reading and reviewing code is one of…

If you read that statement as "when judged on your output by those in control of your salary" does it make more sense which bottom line is in play here?

Re: Code Review Handbook

#16
I also blame the tooling a bit. After using Critique [1] internally at Google all the other tools/implementations/UIs feel very confusing. And I'm not speaking with lack of integration with other tools like coverage, linters, spell checks, error-prone constructs analysis, ... but simply the UI.

Additionally within Google (or at least the teams I was in) really tries to chunk changes in small pieces. For instance, adding a feature always entails something like: 1 CL to introduce the flag that will enable/ disable the feature, at least 1 CL that actually adds the logic and tests, 1 that enables the flag in the deployment. Not sure if that's due to the monorepo or more of a culture thing, but that MO is not that common in my personal experience

[1] https://abseil.io/resources/swe-book/html/ch19.html

Re: Code Review Handbook

#18

I also blame the tooling a bit. After using Critique [1] internally at Google all the other tools/implementations/UIs feel very confusing. And I'm not speaking with lack of integration with other tools like coverage, linters, spell checks, error-prone constructs analysis, ... but simply the UI. Additionally within Google (or at least the teams I was in) really tries to chunk changes in small pieces. For instance, add…

I agree that the tooling is very underrated! It’s strange that programmers spend so much effort choosing their code writing environment (IDE, etc) but just take what they’re given when it comes to code reviewing (mostly GitHub PRs)

If you’re missing Critique, allow me to shamelessly plug CodeApprove: https://codeapprove.com

Just like Critique, it’s a code review tool for power users that really lets you focus on resolving every discussion.

Re: Code Review Handbook

#19
post #15
post #13

It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes. Also, I'd like to push back on this: > Most developers have a lot of features to add and bugs to write, time spent reviewing other people’s work does not add a lot to your bottom line. I disagree; it adds a lot to your bottom line. In fact, reading and reviewing code is one of…

If you read that statement as "when judged on your output by those in control of your salary" does it make more sense which bottom line is in play here?

It depends.

If your employer is a company that values good software development practices, then they wouldn't judge you based on the LOCs you produce, but on the overall value you provide to the team, product and company.

If they're not, then most of what we're discussing here is not very relevant, since they likely don't follow other good practices either.

If you're in that situation, then you have a few options: if you value the product and company, promote these best practices so that the company can attract and keep good developers, which ultimately benefits the product. If you don't, or they won't listen, or you don't have the authority to change things, then you can probably forget about following good practices, and just keep working based on the metrics they do value. Or, I'd strongly suggest, just find a better working environment. :)

Re: Code Review Handbook

#20

I also blame the tooling a bit. After using Critique [1] internally at Google all the other tools/implementations/UIs feel very confusing. And I'm not speaking with lack of integration with other tools like coverage, linters, spell checks, error-prone constructs analysis, ... but simply the UI. Additionally within Google (or at least the teams I was in) really tries to chunk changes in small pieces. For instance, add…

I agree that the tooling is very underrated! It’s strange that programmers spend so much effort choosing their code writing environment (IDE, etc) but just take what they’re given when it comes to code reviewing (mostly GitHub PRs) If you’re missing Critique, allow me to shamelessly plug CodeApprove: https://codeapprove.com Just like Critique, it’s a code review tool for power users that really lets you focus on reso…

Thanks! Will definitely have a look!
Post reply on HN