Hacker News Reader

Comment by hvidgaard | Hacker News Reader

All my developers like the fact that someone else looks at their code. It distributes the responsibility, facilitate knowledge transfer, puts emphasis on readability, ensures that plumbing and "boring" code is idiomatic and plainly result in less bugs.

View on HN

Although I do agree with all of this. I sometimes feel like "distributes the responsibility" can actually cause things to slip through the cracks. The reviewer thinks "eh, I didn't write this, what I saw looked fine" and the author thinks "hey they approved it, so I'm not out on a limb here", causing both sides to look at the code less closely.

I'm not saying that always happens, but it does sometimes in my experience.

One team I used to work on had a really healthy way of dealing with this. Whenever a bug made it through to production, the team would have an informal post-mortem meeting where everyone who worked on the change in question would talk about the cause of the bug, and perhaps try to identify ways something like that could have been avoided.

There was no real interest in assigning blame to individuals. I think that was largely because we required two reviewers to approve every code review. So, when something did happen, it had to make it past at least three people. That automatically makes the mistake a team responsibility and not an individual one.

As a developer, I agree. With perhaps, like the sibling from ’city41, the responsibility. I wrote it, it’s my code. Maybe QA can have their share if responsibility, if any later issue is something QA could have found.

But it requires an environment where responsibility doesn’t equal blame.

This is also one additional reason why I don’t touch Github Copilot. Because I didn’t write that code.

Agreed absolutely.

This is also the sole reason I like sprints. They have storypointing, and that leads to shared understanding and better context switching throughout the sprint.

Yes! Too bad people rarely bother, nobody seems to have the time.

if code reviews come with responsibility sharing then the are going to slow things further, 100% agree with the rest.

How about dealing with "that guy" who will take you through 8 revisions until he basically wrote the PR?

I typically just identify and avoid, get reviews from other people instead.. would love to hear more advanced strats. Attempting to talk about it never works because they dig their heels in on "I'm just enforcing quality".

This is a challenging balance to strike. I used to work with someone who I considered "that guy", but there was no denying that the code ended up better for it, even if it would have been basically fine otherwise. Especially over the long term, those little things can add up.

On the reviewer side, I have sometimes been in the situation where I end up letting something slide because I didn't want to be "that guy". Then I end up reading the code later and needing to fix it.

I think the answer here unfortunately relates to power dynamics. I'm senior, and others believe I'm senior, so when I dismiss reviews like this, no one fights me. Sometimes, with folks like this, even without being senior, just standing up to them works.

As others have said, using linters, auto-formatters, etc often helps with this as a lot of the things these people like to nitpick are then standards enforced by code, and you can send them off to fight people about the standard ("but not in this PR").

Two things, first have a standard to compare to. Second, use code linters and formatters to automatically enforce it.

That obviates trivial back and forth, leaving the interesting issues.

You'd be surprised. Often that dude will have preferences that go way beyond linters and may even contradict widely accepted best practices.

I've worked with the dude who insisted on factoring out functions for unrelated 2-liners, creating extra coupling where we don't want it, the dude who insisted on mocking library code in tests when it would be easier and more thorough to just use it and make sure we're calling it right..

My favorite was the dude who, after 3 days of back and forth changes on a semi-urgent PR goes silent for a day, I ping him, he says "thinking of more comments :)" with the goddamned smiley.

Hah! The simple answer to that however, is "No." With a link to standards/best-practices if you want to be polite.

Unless dude is your boss, in which the answer is "Yes," and the paycheck arrives on Friday.

Also, sometimes other people are right... something to remind myself of. ;-)

Yes, or the guy who wants you to mock out database calls because it's not a "real unit test" otherwise. So you spend another day mocking out crap when it's both easier and less error-prone to use the real thing. I guess the time spent will pay for itself... in 10 years.

The point here is to be able to run the tests without a database, and in this case they're probably right. Saying it's for the sake of purity of the test type is wrong, but you should mock out database calls in unit tests because you want them to run fast and without dependencies.

Move those tests to integration...

We don’t run tests without a database. Never been anywhere that did. I greatly prefer integration tests, in general. They almost always have value.

I dislike code reviews because there is always some guy deciding how things should be. Let's not pretend that the developer has any special say in how the code should look, even though he wrote it. It can be highly demotivating to work as a programmer in such teams.

Usually you have a few guys deciding everything.

90% of people are fine and code review has tons of benefits.. but the format is an enabler for 'that guy', as you're asking them for approval rather than running it by them as an FYI.

I've seen a few different motivated hard workers get to hard work on demanding as many changes as possible whenever they are on a CR.

There is also the flip side too where a block of code might just have a lot of problems and I’m trying to evaluate if it’s too hard in a public forum to flag all of them at once. If possible I try to flag a couple and to sit down and offer to do a one on one for more.

+1 to taking it offline when it's more than a couple minor style fixes.

I think it all comes down to judgement and there are definitely ways people can write code in their individual style without hurting maintainability.

That said, maintaining a codebase where there is a lot of variation in idioms, style and formatting can be a big headache.

It adds cognitive overhead and if you're changing files you constantly have to balance preserving the local style with other factors. E.g. if someone likes aligning their equal signs, and you add a new line where the LHS is one character longer, do you reformat the whole block?

I can't fathom to be honest to be back in a place where something like that is even a question.

We use spotless. We use prettier. Nothing passes the build if you don't adhere to the style rules. You may disagree with some stuff. Make a case! Change the rule definition, apply them and show the PR to a representative cut of the developer community. You might get it through. We've done a few like that in my time here. Or you might realize what that rule change really means in some cases and the PR is ultimately declined. Has also happened. Ruled looked good for the place that triggered the want. Sucked in most other places, so we decided not to do it.

If you have a guy that can't live with not getting his will 100% of the time my advice is to try and get rid of him as soon as you can. Or run yourself. Not healthy in the long run and frankly "I'm too old for that shit. Now get off my lawn"

Reply on news.ycombinator.com