Live data from Hacker News

Code Review Handbook

sledgeworx.io

41–50 of 51 posts

Re: Code Review Handbook

#41
post #29
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…

> 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. Actually, that's not true at all. Code review is not a replacement for automated builds and testing. That's what Continuous Integration is for. Let humans concentrate on what they do best (understanding and comment on code) and don't lose their time and patience with things…

> Code review is not a replacement for automated builds and testing.

I agree, and never said it was. But the fact is most projects don't have complete test coverage, or even replicate the real world environment of actually using the program. In the absolute best case scenario where they do, I agree with you; automated tests and CI should find any functional issues. But if your software is meant to be used by a human, then at some point it should also be tested by one. Preferably before your actual users test it for you.

It depends on the project, of course, so if all you're working on is a library or API consumed by machines, then by all means, rely entirely on CI if you can. Same goes if your project is a small piece of a much larger machinery, and it's difficult to test locally, then it makes sense to rely more on CI in that case as well.

But you can't expect humans to actually interpret all the nuance and complexity of a non-trivial code change from just staring at patches. At least, I'm not that smart. So I typically pull the changes locally, go through the relevant files, try to manipulate inputs and see what the effects are (i.e. manual fuzzing), compare the behavior before and after the changes, and, finally, since I already have the code locally, build and run the program as a last quick check that things actually work. Sure, this is time-consuming sometimes, but it's been helpful for finding issues or just understanding the code more than if I wouldn't do this.

Re: Code Review Handbook

#42
post #25
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…

I went ~25yrs into my coding career without code review and without tests. I wrote games, several AAA games, play testers found bugs, but otherwise there was no code based tests and no code review, none, zero, zilch. From 1983 to around 2008. Now I use tests and code review and I like them. I like that reviewers catch my bugs. I like that they suggest better solutions I didn't think of. I like that they tell me about…

While I think code reviews are great, I think the ideal state is doing them asynchronously post-merge. Enforced pre-merge reviews are a way to hold the change hostage to force a certain quality bar. If there is the experience and trust on the team, you don't need to hold each other hostage. You know the author won't make too much of a mess, you know someone will come around to doing the review soon, getting the benefits of code review, and you know the author will be responsive to feedback and not just dismiss them because they moved on.

Even when you do have pre-merge reviews, some times the culture and tooling can make things slower than they need to be. I saw this at one company:

- Review system sent so many notifications, people ignored them

- It was unclear if a change was relevant for you to review

- When there is communal ownershipg no one owns it. The code review system needs to assign the reviewer from among the team

- it was unclear when you owned reviewing something

- slow CI and not having auto-merge makes people lose track of things. As a last resort consider a two stage CI, fast build of a subset for PR updates, full build on merge.

- people should treat CI as another reviewer and not block on it

Re: Code Review Handbook

#43

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…

Well, you can decide more or less by yourself what tools you use for writing code, but the code review tool is something everyone needs to agree upon (and then convince management to change), so of course it has more inertia. I'd love to use something else than Jira for issue tracking too, but if the whole company uses it...

Re: Code Review Handbook

#44
post #28
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…

I think another factor here is if the reviewer is working on the same code base and the project has a complex environment to set up (e.g., a library compiled with custom flags), then `git stash` may not be enough to checkout the branch and run tests. (I am unfortunately guilty of creating some projects like that myself...) Docker can help with these cases, but not all projects are docker-oriented. I personally like r…

Yeah, reality: no one is going to download code and try to run it. It would take them all day to do code reviews. You are supposed to go live with your buggy shit soon, that's what the business values. Once it's live it gets tossed over the wall and the maintenance process begins where lower paid workers tinker with it until they have the skills to code-and-bail also.

Re: Code Review Handbook

#46
post #41
post #29

Earlier quoted context omitted.

> 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. Actually, that's not true at all. Code review is not a replacement for automated builds and testing. That's what Continuous Integration is for. Let humans concentrate on what they do best (understanding and comment on code) and don't lose their time and patience with things…

> Code review is not a replacement for automated builds and testing. I agree, and never said it was. But the fact is most projects don't have complete test coverage, or even replicate the real world environment of actually using the program. In the absolute best case scenario where they do, I agree with you; automated tests and CI should find any functional issues. But if your software is meant to be used by a human,…

I expect the submitter to have done that. The most annoyed I get in code reviews when it is clear that the tests are green but the code is totally broken and the submitter has never bothered to test it. And at my old job there were things that to properly do a manual QA would have taken all day because it wasn't possible to test on a laptop (e.g. wrong O/S and no ability to easily spin up an AIX image to start with). Really feel like this needs to not be a job of the reviewer, but needs to be on the submitter, but a lot of junior devs or open source contributors tend to skip that step and treat green tests as the definition of done, rather than working software.

Re: Code Review Handbook

#48

I tried to get code reviews to be done after merge. Nobody wanted to do it.

and if you do do it (speaking from experience) people get offended that someone be looking at their code (as if existence in source control guaranteed absence of bugs...). It is all the more surprising since simple inspection of never-reviewed code usually has a lot of low-hanging fruit in terms of bugs.

Re: Code Review Handbook

#50
post #43

Earlier quoted context omitted.

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…

Well, you can decide more or less by yourself what tools you use for writing code, but the code review tool is something everyone needs to agree upon (and then convince management to change), so of course it has more inertia. I'd love to use something else than Jira for issue tracking too, but if the whole company uses it...

What would you go with instead of Jira? At a small startup here so there's still hope
Post reply on HN