There is a balance to be found between pointing out what is going to cause problems and what you find personally inelegant. What I find personally inelegant I typically leave as a suggestion, while approving the MR as a whole. The contributor - who has spent more time than me thinking about his code in the context of the issue - can finish things up on their own, taking my feedback into account where they see fit. Th…
> what you find personally inelegant.
I call this "reviewing for accent". Like, it probably doesn't matter if you use !!foo or Boolean(foo), we all know what they both mean.
I disagree with the title but found myself agreeing with many points in the article. “Don’t be condescending” seems like generally applicable advice. But IMO, sometimes you just know something the code submitter doesn’t (or vise versa) and discussing that can be useful. And i think that’s pretty much teaching!
Yeah, I think the title is a little provocative to get you to read, but I ended up agreeing with it. Maybe "don't try to be a teacher during code reviews" is slightly more precise?
I've had instances of junior people who are simply not interested in learning and suffer from a massively inflated sense of ability and seniority, in practically every job. When reviewing code submitted by such people, "don't teach during code reviews" is actually good advice to the senior person. The senior person is saved the angst and futility of the effort. As a consequence, the other benefit of the code review,…
> Where else will people get habituated […]? Standards documentation, code labs, pair programming, instructor-led group trainings. I agree that code review is a necessary piece of the puzzle, but there are other places for engineers to acclimate too.
Aside from the first, which is almost always out of date, the rest have been nonexistent at every company I have worked. And I do not even know what 'code labs' even means. My experience is fairly typical SV stuff, so I am wondering where these companies that still do all these things are.
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.
> 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. One of them is right, because a codebase that consistently uses one or the other is better than a codebase that mixes the two. If the team hasn't made and communicated a decision then that s…
That doesn't really seem like the sort of thing that needs to be standardized at all, though. They're both valid, they're both readable to anyone competent, just do whatever feels right.
> The Socratic method for teaching is actually quite good, and it can be employed without condescension. Socratic Method is hard to use without coming off as patronizing, even if you think you're being careful. If you're genuinely jumping in to ask real questions to understand the problem, that's great. Most of the time when I see people use Socratic Method in the workplace it's because they think they're doing the o…
+1 I gave up on the "teaching" years ago, it never did anyone any good and wasted a lot of time. Now I say explicitly what I think is wrong and what needs changing. This too can come off badly so I limit reviews to one or two remarks. I also build a relationship with these engineers and explicitly explain how I do reviews different nothing that while I give you solid direction, whether you take my advice is entirely…
I should also clarify that I am always clear on exactly why I am suggesting something, and the level of importance I place on the change (though usually, if I'm commenting, it's relatively important, if not, I just don't bother mentioning it)
Isn’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.
I think it's more of the author having a misguided opinion of what teaching is. I've had a lot of good teachers in my upbringing but not one of them has ever thought that a vague/condescending question like " I’m not sure if I understand the whole idea but could you explain what this method does? " would be an effective teaching method. Don't make teachers the punching bag for bad programmer code review. Programmers…
I think the title wasnt totally clear but it's a totally valid use of the word. Its making the distinction between telling someone the answer and leading them to an answer.
Because to become teachers, you typically don’t have to have your ego brutalized jumping through irreverent hoops. I mean going through engineering school and rigorous STEM degrees I can say that stuff is baked into the formula. You’re derided and dogged and gaslit from the onset. Is it surprising these people graduate, become senior and perpetuate the mental unhealth?
> Because to become teachers, you typically don’t have to have your ego brutalized jumping through irreverent hoops. No, instead you have your ego brutalized by spending half your youth (not to mention tens or even hundreds of thousands of dollars) getting an undergraduate and master's degree and teaching certification...only to receive poverty wages, pay for your own supplies, be abused by students and parents and a…
What kind of idiot would choose that life for themselves?
I've had instances of junior people who are simply not interested in learning and suffer from a massively inflated sense of ability and seniority, in practically every job. When reviewing code submitted by such people, "don't teach during code reviews" is actually good advice to the senior person. The senior person is saved the angst and futility of the effort. As a consequence, the other benefit of the code review,…
> I've had instances of junior people who are simply not interested in learning and suffer from a massively inflated sense of ability and seniority, in practically every job.
Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change. My job is to ship software, and as an engineer/team lead it is my job to ensure the code we ship ( I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line and when I work with peers or more experience people, I barely leav…
I most recently worked on a 100% code-review team. Fabulous! In my personal experience, the hump from wide-eyed novice junior to productive contributing member really doesn't take that long when every line that everyone writes is code-reviewed. The first couple of code reviews might take longer than usual. But it definitely takes much less time than it used to take to bring a junior up to speed. No net productivity i…
It's saying don't be roundabout trying to play teacher in code reviews and other stuff. IMO code reviews often drag on longer than they ought to, so be direct and clear offering additional clarification az needed. Sometime even use Slack or video meet if that clears things up quicker and only record the outcome on the review.
Respect people and their time. Also be open to sugestions and for reviewers not every suggestion is a must, make clear which ones are.