Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Ask HN: What tone to use in code review suggestions?

news.ycombinator.com

Post reply on HN
Print view View original

301–309 of 309 posts

Re: Ask HN: What tone to use in code review suggestions?

#301

Post by clintonwoo »

I'm not sure if the question asked is even the right question (what tone to use isn't the problem). The code review should educate the developer submitting it if they don't know the points you are making. If they do know the points, I would tell them to review their own PR before another person since it helps everybody. We shouldn't be asking for approval for code that's not even double checked.. that throws the responsibility for catching bugs on the reviewer and not even the writer.

If they already self reviewed the code and it has problems, you can write a more detailed message explaining why there's a better way to do things. This way it doesn't come off negatively because the comment is a constructive suggestion. You may also have to compromise some lower priority feedback for development velocity as long as the submitter understands and can improve in the future.

Re: Ask HN: What tone to use in code review suggestions?

#302

Post by DaSpood »

thewebcount wrote: post #187
hcarvalhoalves wrote: post #41

My rules for reviews (which influence the language used) are: * Criticise the code, not the author ("There's a bug here" vs. "You inserted a bug here"); * Reach common agreement that people shouldn't cling to code written ("sunk cost fallacy"). Code is liability, and it's okay to throw things away and restart from scratch, the value is in the learning; * Don't use questions that invite discussion if you're not inviti…

> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools; I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect…

Something as simple as a pre-commit hook that checks files that were changed in your commit and rejects the commit if the rules aren't followed is a very cheap and effective way to enforce a coding style.

For a python project for example, you could set pre-commit to check formatting, linting and typing, which means that when your code reaches the remote repository, before any CI is even started or any PR is openned (before you even push really), you know your code follows the rules that have been set for the project. Thus everyone knows you don't need to spend time in the code review looking for these mistakes and instead focus on the actual features.

pre-commit does not even need to be pushed to remote to work, so if your company has no plan to enforce it but it still helps you, you can still use it, unlike a CI.

As for this: > And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!

It's true that one mistake is easy to fix later, but it's just as easy to fix now. If you postpone it once you will postpone it again when it's not highlighted in a PR diff, it's just a recipe for never fixing it. Every language has automatic linters/formaters that check (and even fix) your code according to rules you define, turning the work of finding these mistakes into a simple `lint ./src/` command. It's takes a small amount of time to setup but saves you a lot of time down the line.

Re: Ask HN: What tone to use in code review suggestions?

#303

Post by thewebcount »

DaSpood wrote: post #302
thewebcount wrote: post #187

Earlier quoted context omitted.

> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools; I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect…

Something as simple as a pre-commit hook that checks files that were changed in your commit and rejects the commit if the rules aren't followed is a very cheap and effective way to enforce a coding style. For a python project for example, you could set pre-commit to check formatting, linting and typing, which means that when your code reaches the remote repository, before any CI is even started or any PR is openned (…

I love the idea of a pre-commit hook. I did actually implement this for a past project, and the rest of the team hated it. Whenever they thought it was being "unfair" to them, I'd get email about it because I wrote it.

The problem with my current project is that it has millions of lines of code and that code all has different styles. So even if you only check the lines that actually changed in a pre-commit hook, you can end up with different styles in the same file, which can be as bad as using the wrong style. At least if it's consistently wrong, working within a single file is doable.

> It's true that one mistake is easy to fix later, but it's just as easy to fix now.

Eh... maybe. I see your point, but I guess what I'm saying is "choose your battles." Every minor thing one points out like this takes time away from me fixing bigger issues, so be judicious in how much of a stickler you want to be for the rules. Yep, I can fix it now pretty easily, and then I won't be working on the next bug until it's fixed. And the way we have GitHub set up, if I make any change, I have to get at least 1-3 reviewers to re-review the code and re-approve it. That's the real time waster because everyone else if fixing the nit-picky things for their latest pull request. If they take too long, then I have merge conflicts and I have to address those before I can commit, and, oops! that causes the review clock to start over. We get to do it all again! Talk about Kafka-esque.

If we were starting a new project, I would probably demand that we start with some sort of pre-commit formatter that forces the appropriate style onto our code, but we're unlikely to be doing that for the foreseeable future, unfortunately.

Re: Ask HN: What tone to use in code review suggestions?

#304

Post by thewebcount »

HWR_14 wrote: post #244
mason55 wrote: post #223

Earlier quoted context omitted.

Everyone is talking about how to get the style guide integrated into your CI. But it sounds to me like your concern is that other reviewers frequently point out your style guide violations. You could set up some local lint rules just for yourself. You'd need to set it up to only run against code that you changed. And if you don't have an automated style guide you'd have to come up with your own. But it seems like onc…

Their concern isn't that other reviewers point it out. It's that they think they shouldn't have to follow the guidelines: > My natural tendency is to do it one way, but our style guidelines prefer it the other way Which is fine if it's a reason it's an occasionally slip up. But apparently, in spite of knowing this tendency, they frequently submit code to review where they've messed up to such a degree that reading ev…

Well that's a very uncharitable way to read what I wrote.

I previously worked on code bases where it had to be one way. It became pretty engrained in me. Now it needs to be the other way. I try my best to use the "new" style now, but because I'm human and busy, I sometimes forget. I spend most of my time focusing on the problem I'm working on and not how I'm placing characters on the line.

I'm not the only one, so I don't think I'm being a particularly bad programmer. We have one reviewer who's really concerned about star placement. It's sometimes the only thing he points out, and he points it out on a lot of different people's work, so I'm certainly not worse than anyone else on the team. As I say, I do try, but sometimes I forget.

The other thing is sometimes I didn't write the code in question. I hit the issue where I'm refactoring and I move a chunk of previously-written code that didn't follow the guidelines, but I didn't modify the code I moved, so now it looks like I "forgot" but in reality, I just moved some code around. How much time do we want to spend reformatting code instead of working on the actual problem we're trying to fix or implement? I would love if we could automate this, but see my other comment about the issues with that.

I think these are all understandable things that busy people have to deal with. It's not that I don't think I shouldn't have to follow the rules. It's that some rules are much more important than others, and differences in code that have the same readability but don't in any way change the functionality seem less important to me than things that can actually cause bugs.

Re: Ask HN: What tone to use in code review suggestions?

#305

Post by thewebcount »

0xffff2 wrote: post #199
thewebcount wrote: post #187

Earlier quoted context omitted.

> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools; I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect…

Sorry, but formatting is on you. If you don't like the comments, don't commit inconsistent code. Set up a local auto-formatter with the right rules. If enough of the existing code you're working on doesn't conform, set the formatter to only run manually and get in the habit of running it before committing.

Yeah, I've been looking for something to do that, but haven't found something that fits into my workflow yet.

Re: Ask HN: What tone to use in code review suggestions?

#306

Post by thewebcount »

physicsguy wrote: post #194
thewebcount wrote: post #187

Earlier quoted context omitted.

> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools; I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect…

Tools are easy. You add it to the CI/CD pipeline, it fails if it doesn’t meet the guideline, people have to run the tool to get the merge in. On first run it’s painful, so if you’re not starting a project, use an autoformatter so it’s not all manual work. If you really want, you can even make your CI pipeline push commits with autofixes Of course if you don’t have a CI pipeline that’s easy to integrate steps like thi…

I don't have the permissions to add it to the CI/CD pipeline. I've filed bugs on our CI/CD to add it, but it's apparently not a high priority (you know, until someone sits down to review code, then suddenly it's the most important thing in the world).

Re: Ask HN: What tone to use in code review suggestions?

#308

Post by physicsguy »

thewebcount wrote: post #306
physicsguy wrote: post #194

Earlier quoted context omitted.

Tools are easy. You add it to the CI/CD pipeline, it fails if it doesn’t meet the guideline, people have to run the tool to get the merge in. On first run it’s painful, so if you’re not starting a project, use an autoformatter so it’s not all manual work. If you really want, you can even make your CI pipeline push commits with autofixes Of course if you don’t have a CI pipeline that’s easy to integrate steps like thi…

I don't have the permissions to add it to the CI/CD pipeline. I've filed bugs on our CI/CD to add it, but it's apparently not a high priority (you know, until someone sits down to review code, then suddenly it's the most important thing in the world).

If I were you, I’d be looking to move on from your company then. Developers should own the testing part of the pipeline at the very least.
Post reply on HN