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

Don’t teach during code reviews

michaelagreiler.com

Post reply on HN
Print view View original

151–160 of 304 posts

Re: Don’t teach during code reviews

#151

Post by sidlls »

Ensorceled wrote: post #85
sidlls wrote: post #68

Earlier quoted context omitted.

"Don't let the perfect be the enemy of the good" is a fine axiom to work by, but like anything else taken to an extreme it can easily be detrimental.

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.

I just strongly disagree with this. The context of a code review is the perfect opportunity to teach, much like a technical design review is for system designs, and having them sit as an observer during an incident response is for triage/debugging a live system under stress. There are no better settings for teaching these skills to juniors. And you should really let your senior engineers take advantage of these times to impart their wisdom. It will help them grow junior engineers into high quality seniors that will help make your team’s output better and faster in the long run.

Re: Don’t teach during code reviews

#152

Post by Ensorceled »

makeitdouble wrote: post #134
Ensorceled wrote: post #65

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…

> it's especially gauling if the original PR was working, reasonably well written, no major flaws [...] wasn't "best practices". You see a clear dichotomy between well written code and what your senior engieers see as enforceable best practices. I'd guess you also see half of the time your team spends on coding as some useless nitpicking you're letting them get away with by sheer generousity or bcause you don't have…

> You see a clear dichotomy between well written code and what your senior engieers see as enforceable best practices.

No, in general, I agree with code review comments my senior developers make. I'm talking about the difference between "best practices" and actual best practices or in the difference between "this works, but next time, a better architecture would be x, let's refactor next time we come back to this" and "I'm blowing up the sprint"

Re: Don’t teach during code reviews

#153

Post by sidlls »

throwaway892238 wrote: post #62
sidlls wrote: post #24

I use junior engineers’ code review submissions as an opportunity to teach, and any senior engineer who doesn’t is committing professional malpractice. They have to learn, and that requires someone (in the case of a code review) to teach them. One can do it without being condescending, though The Socratic method for teaching is actually quite good, and it can be employed without condescension.

Ask them if they want a lesson first. Not every junior wants their superiors lecturing them.

A junior engineer who doesn’t want to learn from a senior engineer is an engineer setting himself up for a failed career.

Re: Don’t teach during code reviews

#154

Post by lmm »

Ensorceled wrote: post #104
bt4u wrote: post #74

Earlier 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.

> 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 sounds like a management failing.

(list comprehensions are the actual right thing to use, for the record, but that's far less important than having a standard and sticking to it)

Re: Don’t teach during code reviews

#155

Post by strix_varius »

Ensorceled wrote: post #85
sidlls wrote: post #68

Earlier quoted context omitted.

"Don't let the perfect be the enemy of the good" is a fine axiom to work by, but like anything else taken to an extreme it can easily be detrimental.

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.

> There are better places, and ways, to teach junior developers than long, painful code review processes.

If I had to choose the best single way to grow as a software engineer, it would be through code reviews: both giving and receiving.

That said, "long, painful" code reviews is a red flag for your team. A code review should have one of four outcomes:

1. Lgtm.

2. Approved. Left some suggestions for your consideration.

3. Specific changes requested.

4. This whole approach needs to be re-evaluated, let's talk.

_None_ of those should be long or painful. It sounds like your team might be doing #4 in the PR itself when really that's an exceptional case that should make it to a usually-synchronous discussion.

Re: Don’t teach during code reviews

#156

Post by endtime »

throwaway892238 wrote: post #61

The worst thing during code reviews is people nit-picking my code or making comments that aren't important questions or mandatory changes. I didn't post this code review to get your two cents! I need to ship this code! Either give it a thumbs up, ask a relevant/important question with context, or ask me to change something that needs to be changed. Otherwise, shut up. I can handle nit-picks, compliments, curiosities,…

I have a very different perspective. I have more than a decade of experience submitting code at Google, and I recently wrote my first PR in a new language. My reviewer knew that language well and his PR was full of nitpicks, and it was great. I learned a few things, and it helped me write cleaner code the next time around.

Re: Don’t teach during code reviews

#157

Post by Ensorceled »

sidlls wrote: post #151
Ensorceled wrote: post #85

Earlier 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.

I just strongly disagree with this. The context of a code review is the perfect opportunity to teach, much like a technical design review is for system designs, and having them sit as an observer during an incident response is for triage/debugging a live system under stress. There are no better settings for teaching these skills to juniors. And you should really let your senior engineers take advantage of these times…

> The context of a code review is the perfect opportunity to teach

I think we are just talking about different scales and different senior developers. Every code review is either teaching something, finding missed bugs, find better ways to do things that a senior developer makes full use of.

If not, your code review is just a preformative waste of time.

I'm talking about simple PRs, the kind I would assign to a junior developer, that are actually finished, getting held up while a "teachable moment" unfolds.

Re: Don’t teach during code reviews

#158

Post by projectazorian »

azov wrote: post #136
Ensorceled wrote: post #65

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…

Well, here is one way to read your comment: "A feature not making it into a sprint" is your problem. You will get in trouble for that, it will make you look bad in front of your boss, etc. "Not following best practices" is not your problem. If the code turns into a mess, it's not you who will have to deal with the consequences, at least not you directly. As a manager you will demand that your senior developers fix th…

> "A feature not making it into a sprint" is your problem. You will get in trouble for that, it will make you look bad in front of your boss, etc. "Not following best practices" is not your problem.

Hmm, almost like this concept of “sprints” is unhelpful.

It’s rare that delaying a PR by 24 hours to get it right has any relevance to business outcomes, in fact it is often better for everyone in the long run.

But with artificial deadlines and fake metrics imposed by scrum, when this happens managers will often freak out because something didn’t “make it into the sprint,” or an engineer’s velocity dropped.

Re: Don’t teach during code reviews

#159

Post by chiefalchemist »

gtirloni wrote: post #9
chiefalchemist wrote: post #2

> I’m not sure if I understand the whole idea but could you explain what this method does? Maybe...It's going to depend on the culture. If you have a passive aggressive culture, something like this is a good fit. Otherwise, to me, you're adding friction. Yes, it might be better to make the submitter think. But if you have to be anti-truth-seek to do it, that's a net loss.

> If you have a passive aggressive culture, something like this is a good fit. That would perpetuate the passive aggressive culture.

That's exactly my point.
Post reply on HN