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

I ruin developers’ lives with my code reviews and I'm sorry

habr.com

Post reply on HN
Print view View original

151–160 of 164 posts

Re: I ruin developers’ lives with my code reviews and I'm sorry

#151

Post by luord »

I strongly disagree with his conclusion, even if I haven't nor ever would be the kind of guy who would just be condescending during a review. No, every mistake during a review must be commented. I just do it politely, as in, point out the error and the possible fix and leave it at that, without any implication about the developer's skills or lack thereof.

That said:

> It can’t be open-sourced or used to lure new developers.

This is a mostly unrelated takeaway that I wish more companies followed so that I could take a look at what I'd be working with when they try to recruit me.

Re: I ruin developers’ lives with my code reviews and I'm sorry

#152

Post by aristophenes »

mirroredfate1 wrote: post #143
tokyodude wrote: post #60

Earlier quoted context omitted.

I can't say I enjoyed code reviewing but I never felt like I was being mean to my teammates. I was really happy when a teammate found a bug in code review of my code rather than that bug getting checked in. I was also really happy when they shared knowledge and I learned something. Maybe they shared a pattern I was unfamiliar with or maybe they pointed out an existing function I could use I didn't know was in the cod…

Bugs are easy.... you just point out the bug and move on... design flaws are hard... it's hard to bring up the fact that while yes, your module solves this one immediate problem, it was written so rigidly that two sprints from now the whole thing will need to be re-written scratch. Or maybe you took a particular approach that seemed easy to implement, but it won't do async stuff properly and so it needs to be re-work…

I had a coworker who had a 2,000 line PR that took me 3 days to review, it was like code breaking. Custom abstractions upon custom abstractions, abstractions in custom imported utilities for a one-liner that was only used once. Basically, instead of writing the code, he made code that generated the code that was needed, once the program was running. Dear Lord. Like somehow this monstrosity was good because instead of creating the feature, he built tooling to make many identical features whenever we want! Except nothing can be different, if it's different we need to rewrite the whole thing.

Every 4 hours or so I would ask my boss if I really had to do it. He told me to just keep going, I think he was building a case for letting him go. I still only made like 10 comments, needless to say they weren't taken well. What a nightmare.

Tangential to this whole thread, but guys like that, every day he worked took two days for other devs to fix/undo his work. Took my company a few years to catch on, he was a senior dev and also good at talking himself up. I would have paid him the same salary to go sit on a beach somewhere and enjoy himself instead of touching the code, we'd all have been happier. In fact the world would be a better place, because now he is presumably working somewhere else doing the same things.

Re: I ruin developers’ lives with my code reviews and I'm sorry

#153

Post by skybrian »

ensiferum wrote: post #148
skybrian wrote: post #54

As a professional, I got tired of being mean, so I switched to a different review style where I ask questions instead. I would still ask a lot of questions. Then I started to worry that I was coming across as passive-aggressive, that these suggestions would be taken as veiled demands to fix things. As a countermeasure, sometimes I'd explicitly say things like, "you can submit this as-is, but here are a bunch of thing…

You don't think that rewriting someone else's patch would be considered rather arrogant? I've worked with a guy who's attitude to most things was "OK, let's submit this for now, I'll rewrite this later". I found that to be rather demoralizing, why would anyone bother to put in their best work when it's just not going to be good enough for this "god of programming". I agree with the approach of asking questions and ma…

Yes, I agree that there is a risk of offending people this way and you need to put some effort into smoothing it over. Programming is often a people problem.

I've only done it a few times where I was the project owner, and I apologized in advance for being overly picky. There was also good reason for it since we were many timezones apart.

Re: I ruin developers’ lives with my code reviews and I'm sorry

#154

Post by skybrian »

mnm1 wrote: post #146
skybrian wrote: post #54

As a professional, I got tired of being mean, so I switched to a different review style where I ask questions instead. I would still ask a lot of questions. Then I started to worry that I was coming across as passive-aggressive, that these suggestions would be taken as veiled demands to fix things. As a countermeasure, sometimes I'd explicitly say things like, "you can submit this as-is, but here are a bunch of thing…

So you want to write each feature/bugfix three times? What kind of company that's still in business does that? I worked with an engineer who would sometimes rewrite my perfectly fine pull requests and it didn't go well. What an absolute waste of time and company resources. Maybe if the code is utter shit and it can't be fixed by the original engineer, you would need to fire him and rewrite it. Otherwise I can't see a…

It depends what you're comparing to. Doing a lot of round trips during code review is extremely costly, particularly when you're separated by many time zones and you only get one round trip a day at best. Getting a patch in can take many days.

Telling someone to change the code (and maybe having them misunderstand) is often less efficient than changing it yourself, due to the extra round trips.

If you're sitting right next to each other, some other technique like pair programming is going to be a lot faster.

Re: I ruin developers’ lives with my code reviews and I'm sorry

#155

Post by mnm1 »

skybrian wrote: post #154
mnm1 wrote: post #146

Earlier quoted context omitted.

So you want to write each feature/bugfix three times? What kind of company that's still in business does that? I worked with an engineer who would sometimes rewrite my perfectly fine pull requests and it didn't go well. What an absolute waste of time and company resources. Maybe if the code is utter shit and it can't be fixed by the original engineer, you would need to fire him and rewrite it. Otherwise I can't see a…

It depends what you're comparing to. Doing a lot of round trips during code review is extremely costly, particularly when you're separated by many time zones and you only get one round trip a day at best. Getting a patch in can take many days. Telling someone to change the code (and maybe having them misunderstand) is often less efficient than changing it yourself, due to the extra round trips. If you're sitting righ…

You bring up a good point about changing the code. I wouldn't want the reviewer changing the code himself and not getting reviewed again. But even if it was reviewed again, I would still find it problematic as the reviewer might make changes that are unnecessary and detrimental to the code base based solely on his opinions. For example, he prefers to use front end templates vs. backend or prefers to break up classes that should stay together, etc. Most code review issues that come up are a matter of opinion and it should be up to the author to change or not to change the code. If the reviewer feels so strongly about a change, he should submit a different PR in addition and it should be evaluated separately. I can't think of anything more enraging than coming in and seeing your reviewer rewriting your perfectly good PR simply because he wants to do it his way. At that point, I wonder why I even wrote a PR in the first place. Why not have this asshole write all the code then?

Re: I ruin developers’ lives with my code reviews and I'm sorry

#156

Post by skybrian »

mnm1 wrote: post #155
skybrian wrote: post #154

Earlier quoted context omitted.

It depends what you're comparing to. Doing a lot of round trips during code review is extremely costly, particularly when you're separated by many time zones and you only get one round trip a day at best. Getting a patch in can take many days. Telling someone to change the code (and maybe having them misunderstand) is often less efficient than changing it yourself, due to the extra round trips. If you're sitting righ…

You bring up a good point about changing the code. I wouldn't want the reviewer changing the code himself and not getting reviewed again. But even if it was reviewed again, I would still find it problematic as the reviewer might make changes that are unnecessary and detrimental to the code base based solely on his opinions. For example, he prefers to use front end templates vs. backend or prefers to break up classes…

Yeah, so in the end it's all about the relationship between the programmers, and as I've said, this can be tricky. If you neglect that it's not going to work.

For example, if you're sending a patch to the owner of the code and they are known to be very picky, maybe it won't be considered an insult to rewrite it, since in the end it's their project and it saves a lot of back and forth? Or maybe it's a team where they have gotten used to it?

Believe it or not, starting from a patch that actually works is actually pretty helpful and does save time compared to starting from scratch.

Re: I ruin developers’ lives with my code reviews and I'm sorry

#157

Post by dennisgorelik »

Code review is a power tool that improves codebase and improves software developers' skills.

If code review breaks software developer's psyche, then this software developer is unlikely to be any good in the first place. How can you be good without ability to listen to code review feedback?

Re: I ruin developers’ lives with my code reviews and I'm sorry

#158

Post by ljm »

finnthehuman wrote: post #28

This guy ties his ego to his code, and assumes everyone does the same. The proposed solution is to not devalue the person by devaluing the code. This seems exactly backwards to me. Disconnecting ego from the work was the first big lesson I had when I started working in software. I hear parallel ideas from friends across industries, in fact an electrical contractor explained to me how he expects it of his apprentices…

I've had my fair share of controversial code reviews, both on the giving and receiving end.

I no longer ask 'why?' inside a code review, because it immediately puts the author in the position of justifying what they did, while feeling like they have to defend a decision.

Instead, I'll take a coaching (or a more socratic) approach that doesn't risk making the author feel dumb, or like they never considered the alternatives: "what was your reason for this?", "did you try x,y,z?", "I wondered if this would work?", "what is this for?", "what does this do?"

In addition to that, if I have a suggestion (or a suqqestion [0]), I will always follow it up with a refactored, copy/pastable code snippet to make it clear what I'm talking about and also to help push the review forward. This is especially useful for learning by example.

This means that code review takes more time and effort, but that investment of time has value. Code reviews become an implicit, shared mentoring space, and people like it when they see your review pop up.

[0] https://www.urbandictionary.com/define.php?term=Suquestion

Re: I ruin developers’ lives with my code reviews and I'm sorry

#159

Post by mnm1 »

skybrian wrote: post #156
mnm1 wrote: post #155

Earlier quoted context omitted.

You bring up a good point about changing the code. I wouldn't want the reviewer changing the code himself and not getting reviewed again. But even if it was reviewed again, I would still find it problematic as the reviewer might make changes that are unnecessary and detrimental to the code base based solely on his opinions. For example, he prefers to use front end templates vs. backend or prefers to break up classes…

Yeah, so in the end it's all about the relationship between the programmers, and as I've said, this can be tricky. If you neglect that it's not going to work. For example, if you're sending a patch to the owner of the code and they are known to be very picky, maybe it won't be considered an insult to rewrite it, since in the end it's their project and it saves a lot of back and forth? Or maybe it's a team where they…

I'm OK with sending a patch or pull request back to the author in any circumstance and using the author's work as a base for said patch/PR makes complete sense to me. The problem I've seen is when the reviewer takes it upon himself to make these changes directly in the current PR without giving the original author a chance to change or defend his code. This is exacerbated when said changes take an entire day or two of the reviewer's time and are so extensive that the original work submitted becomes irrelevant. This wastes the reviewer's time which could be spent on other things instead of rewriting something that already works and has been done and it wastes the author's time because it essentially discards his work. You're right, it is definitely about the relationship between programmers, but I think some ground rules would help in cases where the relationship is unclear or the programmers work async remotely.
Post reply on HN