As a manager, it drives me crazy when some feature doesn't make it into the sprint because some "senior engineer" decides that this is the time to teach a junior/intermediate the proper way to do something with a long drawn out "teaching process" via PR comments. it's especially galling if the original PR was working, reasonably well written, no major flaws and they forced the junior to rewrite because it wasn't "bes…
galling. gauling is what the french do.
Don’t teach during code reviews
171–180 of 304 posts
Re: Don’t teach during code reviews
#172Isn’t that more “don’t give vague feedback” or even “don’t misdirect the reviewee”? “I’m not sure if I understand the whole idea but could you explain what this method does?” misdirects by suggesting the reviewer thinks they need education, rather than that the reviewer thinks the code can be clearer.
Re: Don’t teach during code reviews
#173> I’m not sure if I understand the whole idea but could you explain what this method does? What's wrong with this? The author suggests that this is somehow extremely condescending, and that the reviewer should instead review at an "eye-to-eye level". So the solution is to jump to a remedy without fully understanding what the writer meant? If you are going to review at an eye-to-eye level, then you have to go in assum…
That is why the suggested rephrasing was to say: I had a hard time understanding how the method worked. How about renaming it?
Re: Don’t teach during code reviews
#174Earlier quoted context omitted.
Frankly I think everyone should have this attitude, seniors as well. (I try to) I am experienced but I know I write bugs and code that may be not as clear as it could be. We all do: we're only human. Also depending on the codebase (and language to some degree), if someone senior writes a lot of overcomplex or abstract code that the rest of the team can't understand/maintain, that's just as much a problem as anything…
> if someone senior writes a lot of overcomplex or abstract code For me this is a much bigger concern than subtle changes to method names. My biggest headache digging into new codebases is when I run into layers and layers of abstraction that save 3 lines of code but force me to construct an entire mental map of the codebase before I can understand how anything works. Any tips for giving feedback to senior people her…
Like dependency injection is something that a lot of juniors struggle to understand. Or in larger applications DDD and all the crap that goes along with it.
Re: Don’t teach during code reviews
#175As a manager, it drives me crazy when some feature doesn't make it into the sprint because some "senior engineer" decides that this is the time to teach a junior/intermediate the proper way to do something with a long drawn out "teaching process" via PR comments. it's especially galling if the original PR was working, reasonably well written, no major flaws and they forced the junior to rewrite because it wasn't "bes…
Real world example: a company I consulted for a couple years ago had a lead architect known for nit-picking code issues with junior devs ad nauseam (to the point where potential hires were told to expect it and not join if they didn't think they could take it). Their production system was brought to their knees by a doubled for loop, literally just a duplicated line of code. This code copied messages from one place to another, but almost all of their testing used cardinality 1 so it didn't affect the tests. In production all it took was a couple cardinality 2 cases because there was one edge case flow that could route copied messages back through the loop and this caused cascading failures as 2 became 4, 4 became 16, etc. This error would have been obvious to anybody reviewing the code, even if they didn't really know much about it, but as that module was written by said architect no one else felt qualified to review it.
Re: Don’t teach during code reviews
#176The proposed approach to code review in the mentioned medium article makes my skin crawl. So patronising and passive-aggressive.
I've had people tell me they think its insane how some people can't handle code reviews and would go to their manager about it.
Re: Don’t teach during code reviews
#177> I’m not sure if I understand the whole idea but could you explain what this method does? What's wrong with this? The author suggests that this is somehow extremely condescending, and that the reviewer should instead review at an "eye-to-eye level". So the solution is to jump to a remedy without fully understanding what the writer meant? If you are going to review at an eye-to-eye level, then you have to go in assum…
Re: Don’t teach during code reviews
#178> I’m not sure if I understand the whole idea but could you explain what this method does? What's wrong with this? The author suggests that this is somehow extremely condescending, and that the reviewer should instead review at an "eye-to-eye level". So the solution is to jump to a remedy without fully understanding what the writer meant? If you are going to review at an eye-to-eye level, then you have to go in assum…
Re: Don’t teach during code reviews
#179Earlier quoted context omitted.
You have to fight to produce quality and you have to do it continuously. You cannot just merge in "working(tm) but not using best practices" and then turn on the quality-switch months or years later. Your attitude is creating a culture where nobody cares and it's why many good engineers end up hating their job.
Wow. You sure are extrapolating a lot about me from a simple comment. While we're extrapolating, are you the type that makes junior developers rewrite their loops as list comprehensions or the type that makes them rewrite their list comprehensions as loops? Because I've seen both in code reviews.
There is not going to be one answer for every situation. Let's say your startup runs out of money tomorrow, the feature must be demo'd today to raise more money, is this code getting merged or debated? If this software for a life support system the bar is set very differently. What's the cost of failures, what's the cost of future maintenance, etc. - all matters.
If the senior engineer has enough projects/years under his belt, good judgment, has seen various business outcomes, and can weigh this, then I would generally trust them as being closest to the decision point. If those are the senior engineers on your team I don't think code reviews and mentoring juniors is going to be a problem.
Re: Don’t teach during code reviews
#180Earlier quoted context omitted.
Sure, we have code reviews for a reason, to make a better product. There are better places, and ways, to teach junior developers than long, painful code review processes.
Code review is the most common arena for getting feedback on my code. My team doesn't pair very often, though I think that's more effective. I don't believe I have other opportunities for it...?