4 ms·

> tagging the author Sounds like trouble to me. “Hey I rewrote what you wrote because it sucked. Got problems with that?” I suppose the best solution to this

by lakpan 3y ago

> tagging the author Sounds like trouble to me. “Hey I rewrote what you wrote because it sucked. Got problems with that?” I suppose the best solution to this problem would be a “late review” asking for an explanation, offering a solution, and hoping it doesn’t just get ignored, which is the easiest thing to do for the author.

viraptor 3y ago
> Hey I rewrote what you wrote because it sucked. Got problems with that? Reading it like that is just insecurity speaking. If anyone wants to rewrite my code to be more readable, I'd love for them to tag me in the review and show me how they did it, so I can learn. Assuming good intentions and being open to discussion goes a long way :) I'll always tag the author for review if I'm rewriting something recent. It's more "Hey, I was too late to mention this in the review, but I think this improvement was worth the time. Let me know what you think in case I missed something important about the original."
- lakpan 3y ago
  > I'd love for them to tag me That’s generally what I like too, but often in teams my suggestions are either approved with zero comments, ignored or followed by wasteful reviews by people who don’t know the language they use every day, and don’t bother to google their answers.
oytis 3y ago
I don't think a minor deduplication justifies a separate discussion even before a single line is changed. That would make a huge deal out of it and waste everyone's time. I wouldn't probably make a separate PR for that - just refactor the code next time I need to touch it - and tag the author for review of course.
JonChesterfield 3y ago
It's a context thing. The point of adding the author as reviewer is to keep them in the loop, show an alternative solution and discuss which way to go. Also to learn why the did it the copy/paste route in the first place - it might be carefully reasoned, it might be a rush job, they might not know any alternatives.
- developer93 3y ago
  Also to put the conversation on record somewhere so that if someone comes later with the same complaint they can see why the code was written like that.
karmakaze 3y ago
I wouldn't necessarily tag the author for a review. I might ping them to notify that I was doing some refactoring in the area and leave it at that. Of course it all depends on the team size, diversity/range of work, typical interactions between members, etc.
sumtechguy 3y ago
If someone had just summerly re-written my code I would personally go talk to them and found out 'whats up'. As it shows a communications breakdown. Second the person who did it should have approached the other dev and said 'hey I see you have blah why? would not this way be better?'. This happens many times in codebases. People begin to 'own' sections of it and they become very protective of what is going on in their area. They take it very personally if something changes and they were not in on it.

by lakpan 3y ago

viraptor 3y ago

> Hey I rewrote what you wrote because it sucked. Got problems with that? Reading it like that is just insecurity speaking. If anyone wants to rewrite my code to be more readable, I'd love for them to tag me in the review and show me how they did it, so I can learn. Assuming good intentions and being open to discussion goes a long way :) I'll always tag the author for review if I'm rewriting something recent. It's more "Hey, I was too late to mention this in the review, but I think this improvement was worth the time. Let me know what you think in case I missed something important about the original."

lakpan 3y ago
> I'd love for them to tag me That’s generally what I like too, but often in teams my suggestions are either approved with zero comments, ignored or followed by wasteful reviews by people who don’t know the language they use every day, and don’t bother to google their answers.

oytis 3y ago

I don't think a minor deduplication justifies a separate discussion even before a single line is changed. That would make a huge deal out of it and waste everyone's time. I wouldn't probably make a separate PR for that - just refactor the code next time I need to touch it - and tag the author for review of course.

JonChesterfield 3y ago

It's a context thing. The point of adding the author as reviewer is to keep them in the loop, show an alternative solution and discuss which way to go. Also to learn why the did it the copy/paste route in the first place - it might be carefully reasoned, it might be a rush job, they might not know any alternatives.

developer93 3y ago
Also to put the conversation on record somewhere so that if someone comes later with the same complaint they can see why the code was written like that.

karmakaze 3y ago

I wouldn't necessarily tag the author for a review. I might ping them to notify that I was doing some refactoring in the area and leave it at that. Of course it all depends on the team size, diversity/range of work, typical interactions between members, etc.

sumtechguy 3y ago

If someone had just summerly re-written my code I would personally go talk to them and found out 'whats up'. As it shows a communications breakdown. Second the person who did it should have approached the other dev and said 'hey I see you have blah why? would not this way be better?'. This happens many times in codebases. People begin to 'own' sections of it and they become very protective of what is going on in their area. They take it very personally if something changes and they were not in on it.