Hacker News Reader

Comment by Ensorceled | Hacker News Reader

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 "best practices".

Some of the comments here seem more like sabotaging junior developers career by drawing out how long it takes them to get through code reviews than teaching them anything.

View on HN

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 that (while still shipping features, of course). After all, that's what expected of senior developers. So, the incentives for you and for your senior engineers are not aligned.

Of course I don't know if that's even remotely close to your reasoning. I don't know you or your project or your priorities, I have no idea what "best practices" we're talking about and how reasonable it is to follow or not follow them in a given situation. I might be totally wrong, you may have all the right reasons to be mad about those reviews. But the comment makes it sound like your senior engineers are idiots who block PRs for no reason, and that can't be good for your team.

> But the comment makes it sound like your senior engineers are idiots who block PRs for no reason, and that can't be good for your team.

Ah, I see, you think this is a general problem I have. No, in general my senior engineers are excellent and all the current ones I work with are.

I've seen enough of the other, especially while contracting.

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

Yep. At this point "scrum" (or "daily standups", but that's a separate issue), "sprints", or "burndown charts" are all run-for-the-hills red flags to hear about when chatting with companies, IMO. It shows a lean towards process for the sake of process, artificial deadlines and artificially subdividing work to fit within said deadlines, and sometimes a "we do this because everyone else does, without questioning it" cargo-cult mentality that often extends into the tech itself.

There's exceptions, clearly. And the process does work for some folks. I'm glad those people are happy. It has rarely worked for me, or for teams I've been on.

> 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 a choice...From the looks of it that can't be a healthy team dynamic

> 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"

Comments such as “blowing up the sprint” suggest a habit of catastrophizing because an artificial deadline wasn’t met, that’s not going to serve you or your teams well in the long run.

When I hear comments like this from managers it’s generally a sign that I need to start planning an exit strategy.

edit: Also, “my senior developers?” They’re human beings, not your property, dude

Honestly it sounds like a terrible place to work. There’s artificial deadlines with no slack for error, and Junior devs are unable to learn to allow them to be better in the future.

As a manager, the Junior shouldn’t be on the critical path solo. That’s just bad management.

“this works, but next time, a better architecture would be x, let's refactor next time we come back to this”

In my experience there is rarely ever a “next time” and “fix it later” becomes “fix it never”. It’s always cheapest to fix worst practices up front rather than letting them metastasize into a huge pile of technical debt later. It’s also rare to find anybody interested in or willing to go back and fix old stuff when priorities often involve chasing the next shiny thing.

Exactly - this, coupled with phrases like "blowing up the sprint," suggests that nobody on this team is likely to trust that the offer of "fixing it later, next time" will be held to.

My rule is code at inception doesn't have to be the best, but every time you touch existing code you must make it better.

Then the code that gets touched the most gets touched up the most. Code that doesn't ever get touched is fine being kinda crappy.

> the difference between "best practices" and actual best practices

hmm...this would sound so much better if you felt your team was pushing for "actual" best practices.

Perhaps it's all working well, but from your words there's so much underlying tension and your clarification adds further distanciation between you and your team.

I'd hate to be on either side of this to be honest.

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

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

If you're getting complete rearchitecture feedback in a code review, something's wrong because that's too late for it. Especially if it means rewriting later changes they haven't put up yet.

The wrong part happened when sender went to work on something alone for a long time then sent a giant PR all at ones instead of progressive set of changes. Don’t blame the reviewer for having to either compromise their integrity by letting it slide into the codebase or getting onto the likes of TP’s shitlist by putting their foot down.

i disagree, it is the place. The work shouldnt be at risk of missing release to begin with, timelines should not assume a senior dev works the issue.

Like what? What's that Confucius saying: "I hear and I forget, I see and I remember, I do and I understand"? Nothing beats teaching using a real example that the parties are hands on with.

I get what you're saying. Delaying a milestone to have a long convo in a CR is not good prioritization. But as the technical manager it's on you to budget more time for junior devs to land their changes.

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.

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

But "simple PRs" with poor implementations compound over time and make for outsized tech debt in the future. I agree that addressing specific major issues within the PR (e.g. comments) is not a good approach. If there are "enough" (for some relatively subjective definition of that) "teachable" things, it's likely an in-person (or at least chat thread) conversation is warranted. Under normal circumstances (and I'd hope a junior engineer isn't making a PR to patch something for an emergency/incident resolution), a PR should be fine to hold up, as the release timeline should account for the fact that junior engineers will have longer PR cycles.

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

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

Like what? And dont tell me it’s some kind of lecture/lab-style group meeting for 1-2hrs

On the other hand, some people actually write stuff that isn't great but visually seems ok. You have the senior engineer to say 'no lets not create a mess'

Yes. Why do you think I have code reviews if not to stop messes from getting made?

I'm not talking about that.

> reasonably well written, no major flaws and they forced the junior to rewrite because it wasn't "best practices"

People without technical background shouldn't be allowed to be managers of highly technical teams.

I agree. Wait, do you think I'm non-technical?

At least I think so because of your previous comment. Since I can only take this as context their might be a misunderstanding. Still I would like to point out a few thinks, I did not like about your comment.

In my opinion, you are looking at the problem from the wrong angle. If code gets worse (non working) after a code review, the reviewer made some serious mistakes and/or was not guiding his colleague to the correct solution like it should be. Also if working code gets rejected because of bad practice, you should not be mad about it but be thankful that someone caught that before release. If your only measurement of code quality, is that something is working, I would consider that a dangerous practice.

> If your only measurement of code quality, is that something is working, I would consider that a dangerous practice.

I'm really confused as to why there are so many people assuming I don't give a shit about quality when I have CODE REVIEWS.

Why are you assuming I want them all to just be rubber stamps?

And what God Complex do you have to have that you assume the "senior developer" is ALWAYS in the right, and the junior and the "non-technical" manager just don't respect "quality" at all?

Not OP, but your final comment confuses me -

Given a senior developer, a junior developer, and a non-technical manager, in the context of a code review, the vast majority of the time you should absolutely listen to the senior developer.

If that's not true in your organization, then hiring, leveling, and leadership should all be questioned.

I'm a very hands on, technical manager. But thanks for assuming I'm non-technical.

Assumed? I responded to your comment:

> you assume the "senior developer" is ALWAYS in the right, and the junior and the "non-technical" manager...

> I'm really confused as to why there are so many people assuming I don't give a shit about quality when I have CODE REVIEWS.

Having code reviews is an incredibly low bar. The very fact that you offer this as an evidence might be the reason people make those assumptions.

Is it? Their comment might come from having worked at companies where code reviews aren't even a thing, let alone source control. It's crazy out there, especially if you're nowhere near Silicon Valley.

Your comments are very reminiscent of non-technical managers I've known - they often have a very shortsighted view of the value of code reviews, because they don't need to work on the code.

'It works - why don't we just merge it? Keep velocity high!'

A code review is exactly where it's worth spending time making sure: the code is maintainable, doesn't degrade the quality of the repo, and above all teaches the junior things they can use next time to do a better job faster.

Spending some time using a review as a teaching experience pays so many dividends later. People who don't touch the code don't understand that.

Of course, there's a level of 'good enough' a senior should be able to identify and approve. But the bar should be high.

Your and some of the other comments I've read are very reminiscent of some of the worst senior developers I've worked with, because they treat code reviews as a way of molding the code base to their own personal expectations rather than improving it especially when a junior engineer finds fault in their code. They waste time using code reviews as a teaching exercise that gives bad habits to junior developers that need to be broken later.

Why? His comment was the perfect description of what a senior developer should do during a code review. Code reviews between a senior and junior are teaching exercises and always should be. It is critical and a fundamental step for juniors to be become more experienced. As already written by the comment you replied to, the right balance has to be found, but this is part of the job description if you call yourself a senior developer.

> I'm really confused as to why there are so many people assuming I don't give a shit about quality when I have CODE REVIEWS.

Because code reviews are a minimum. This is like saying "Why are there so many people assuming I don't give a shit about driving safety, I put on my seatbelt!" Yeah, it's because you're driving rashly and putting others at risk. Just putting on a seatbelt isn't the be-all and end-all of the process.

If you want to know, the reason that people are assuming you don't give a shit about quality are your sprint-centric attitude, your putting the word 'senior engineer' in double quotes, and the talk about "well if the PR was working then it should ship".

This gives off an impression of someone who doesn't understand at a fundamental level that the process of code reviews and have the contribute to learning on the team is is what makes for quality in the long run, not just the mere fact of having them.

Of course, it's quite possible you are the reasonable party here and your senior engineers are curmudgeons. In that case, have you tried talking to them about it? What did they say? Your complaint here seems to indicate that this hasn't happened yet.

(I'm Not OP) - At the end of the day we're all trying to strike a balance between shipping fast and keeping technical debt down. Asking juniors to only ship code that's good as what a senior would write is very rarely the right balance point. Senior engineers must learn to understand when code is "good enough". If you're a senior and you haven't developed this skill you're bringing your team down.

Concretely, a heuristic I rely on is to understand whether the sub-par code being added "infects" the code base e.g. it changes an important interface, it adds a poorly designed interface other code will rely on etc. These are the places its important to put your foot down. Conversely, if the internals of a one-off function or class could be a little cleaner... eh, ship it.

> Senior engineers must learn to understand when code is "good enough". If you're a senior and you haven't developed this skill you're bringing your team down.

Holy shit. I should have just written this.

I don’t see what here implies the grandparent commenter does not have a technical background.

If you don't want to see that drawn out at code review time, then have the juniors consult with the seniors prior to that stage. If you don't want seniors holding juniors to standards then do away with the junior/senior title separation because it might be meaningless to you.

If I thought the junior was not up to the task I assigned them, I would have had them consult with a senior developer during the task.

I'm literally talking about the case where a senior developer decides that this task was not implemented to their personal standards AND makes it a "teaching moment" that means it doesn't get delivered this sprint.

Saying "this can't ship without significant rework" is something that needs to get escalated, not ground out in a code review.

I'm an engineering manager with a coding background 12 years coding, 3 years managing, and "assigned as task" throws a red flag for me. I believe we should be (a) understanding what preferences and skills each engineers have (b) have a conversation on how they can best make impact using these (always comprises, sometimes a project just needs to be done and they're the only one free, but by that not being the norm, they don't mind), and (c) setting a goal for them to achieve (rather than a "task"). E.g. expose an internal REST endpoint providing these parameters with this latency and scale by two weeks, and letting them determine the tasks needed to get there. (obv this goal should be set together as well, not dictated).

> E.g. expose an internal REST endpoint providing these parameters with this latency and scale by two weeks, and letting them determine the tasks needed to get there.

But that is a task

This absolutely does happen and is a problem. Funny seeing so many replies mischaracterizing what you're saying.

The pile on is interesting. Apparently I’m a terrible, non-technical, micromanager who believes I own my staff and working for me would be a horror show. All because I’ve said some senior engineers go to far in code reviews.

"It works" is perhaps the lowest bar you can possibly have for merging code.

"It works" is the most common metric I've seen juniors decide when to submit code (the rest being just trash). Add code review to that and expect it to get merged because "it works"? Not sure the problem is with the code review holding up the ship...

Ironically, I think the people responding to and freaking out about your response proves the OPs point, that many engineers treat PRs as an exercise in ego stroking rather than reinforcing software design.

When I was a junior engineer, nothing annoyed me more than engineers trying to 'teach' during code reviews and doing so in one of two ways:

1. Being vague and unhelpful under the guise of having me 'figure it out'. They would point to a spot and say 'This needs fixing, rework this', without saying how to rework it other than to figure it out. Eventually I would need to drag them into a call to get them to tell me how they personally wanted it fixed up. Often times the way they wanted it fixed was incorrect because they misunderstood the work or were just wrong.

2. Giving incredibly bad 'improvements' to the code. Things like 'change X loop style to Y loop style' where Y doesn't affect the readability or performance of the code, or things just for the sake of adding comments.

So nowadays when I do PRs for juniors and such I try to have direct, actionable comments with rationale and room for disagreement, or if something needs to be reworked I try to assess why they did it that way and do a quick call/discussion with them. Junior engineers are generally not dumb and too many seniors treat them as such.

I agree to some extent. Personally, I do spend the time to teach in PRs but also approve the PR to unblock unless there's much needed changes. It unblocks people, and you quickly see if they cared about your comments if they follow up with another PR addressing them.

Yes, this is how you have your cake and eat it.

It works in reverse too, when you’re the one being reviewed and you receive feedback you disagree with.

Instead of dying on the hill and delaying your merge, just implement the feedback and then argue. You can make your point while still accommodating your teammate and moving things along.

(Obviously, this practice shouldn’t apply to feedback which is truly bad)

This author, and 99% of anybody writing about, and large majority of anybody attempting to do code reviews misses that there are two very distinct and not particularly compatible purposes of code reviews. IMO the only really important one is identifying code that doesn't do what it's supposed to, either because the author miswrote it or misunderstood the requirement. Critically this is just "what", not how or why. This is often really simple stuff, like getting a sign wrong or storing a value to the wrong field/variable or using && instead of ||. 100% objective stuff. The other purpose I'd more broadly describe as "design review", which is focusing on "how" the code should have been written. In my experience this is mostly subjective and that's what consumes all the time. My suggested approach is to officially delineate the two, and whenever someone raises design or style issues in a code review just say "that sounds like a good topic for the design review". You can actually use PRs for both purposes, just front load the "design review" during the WIP phase for a feature branch, and then do the "code review" before merging into a mainline.

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.

The what vs how vs why is a great distinction. Feedback on how has a place but I hesitate to give a lot of it and tend to make it non blocking.

Sounds like someone trying promotion driven development, instead of doing their job (<<<<<<<) effectively.

Code reviews are important, but I think I know the type of person you are talking about which takes these things to an extreme and ends up being dead weight. Often times, they're not as good as they think either, just being honest.

You need to rein in these people ASAP, honestly they are the worst kind of coworkers. Toxic positivity and wasting time/effort 99% of the time. We're paid for our skills, and one of those skills is being able to figure things out on our own or ask for help with pertinent questions. This hand holding crap to get eyes on you is just silly and something I really dislike working at big corp.

I’ve always thought if it’s a “teaching moment” that it makes more sense to just pair with the engineer and go through it together. Otherwise it’s a really long turnaround to go back and forth in PR comments that just frustrates all parties.

Depending on what you're pointing at, it might be better to let the other party take whatever time they need to digest your comments and adjust (including comming up with counter points)

For instance if you're telling them about a behavior defined in a RFC, it can have better long lasting impact if they get the time to read and understand it on their own, than if you're over their shoulder shoving info at them.

Basically, live teaching requires you to be good at reading the other person's state and providing the relevant info in a digestible way, while they live code. It can work, but that's a high bar to pass.

This is more prevalent than one thinks, particularly at large companies where senior developers will ruthlessly put roadblocks, either as a way of carrying out group politics, or keeping control over things where the risk of a new idea or paradigm exceeds the possible downsides, however minor, of rocking the boat.

However, if this is in a small team, then it's a management problem that needs to be solved by the people manager. Either the hiring process is not eliminating bad performers or people with bad attitudes, or team dynamics have deteriorated considerably.

From the senior's viewpoint, though, if they are going to be held solely responsible for janitorial work to avert juniors' substandard work blowing up down the road, or picking up the pieces when that does happen, then it makes perfect sense for the senior to put up roadblocks, junior dev's career be damned. Again, this is likely a management problem where people are not held accountable and made to maintain systems they come up with (tenure is so short, especially among junior people, that this is likely to be a systemic problem).

Do you talk about your “senior engineers” this way to their faces? Using scare quotes to refer to the titles of your colleagues? Do you refer to them as minions too?

You sound like an absolutely insufferable person to work under.

Agreed. Too many "senior engineers" fail to recognize when they are simply venting their territorialism by nitpicking in code reviews. The role of a good manager is to restrain these tendencies.

galling. gauling is what the french do.

Thanks ... clearly to much Astérix & Obélix.

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.

The vast majority of questions around quality that end up surfacing in code reviews can be much more easily settled by commonly shared tooling that enforces organizational style and security practices. Leaving "best practice" up to the code review stage just ends up allowing senior engineers to demand code that they themselves would write, not what's best for an organization.

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.

I think that these kind of reviews can potentially be squashed by building consensus around what kind of feedback is right for PRs, what conventions you agree upon using, etc. Far too often, these kind of "best practices" only exist in the senior's head (if they're even truly best practices at all), so it becomes a very frustrating moving target for a junior.

I agree you should push back against this as a manager, but it can be hard to do so tactfully from my experiences. You either have to say, "no," or engage in protracted debates on subjective ideas around readability and maintainability.

> I agree you should push back against this as a manager, but it can be hard to do so tactfully from my experiences.

Just review this thread ... it can be utterly painful. There are at least as many senior devs who don't like to take feedback on how to deliver feedback as junior devs who don't like to take feedback.

Why would you rewrite them as loops?

"It's much easier to read a loop than a list comprehension, please rewrite all the list comprehensions as loops."

I squashed that.

Sounds awfully micromanagey, making the decision as someone with way less context than the senior engineer… unless you have OCD and non-pragmatic engineers upholding unreasonable expectations.

List comprehensions are advised against in Google’s official Python Style Guide and many similar guidelines for this exact reason. You sound like a micromanager who thinks he’s more technical than you actually are

> 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)

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.

Mixing and matching them makes the codebase harder to maintain, especially for those juniors who we're supposedly concerned about.

I don't think it does. This isn't some big architectural thing where the choice actually has implications that reach beyond the function you're in. It's not even on the level of naming conventions, where you might have to go look elsewhere to see how something is named. If you can understand both of them, which you should be able to even as a junior (as you even imply yourself, by saying it doesn't matter which one you pick), you can read both of them just fine.

> It's not even on the level of naming conventions, where you might have to go look elsewhere to see how something is named.

It's similar to naming conventions - you wouldn't want to mix snake_case and camelCase in the same code - only more so, because it's a bigger leap from one to another. Things that are the same should look the same and things that are different should look different. If each loop in your program is represented differently, it'll be hard to understand.

> If you can understand both of them, which you should be able to even as a junior (as you even imply yourself, by saying it doesn't matter which one you pick)

Juniors can learn anything but they can't learn everything. The more trivia they have to deal with, the longer it will take to learn the big things. It doesn't matter in the same way that, say, it doesn't matter whether you pick Rails/Ruby or Python/Django for your webapp; it's still a bad idea to do both at the same time.

One problem is software developers losing sight of business goals. The business questions include: What's the cost of that code merging as is. What's the cost of the merge being delayed and the feature not being shipped. What are the different options that we can pursue to maximize the value of the business. This is going to be my problem or someone else's problem is not the way to make good decisions.

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.

Reply on news.ycombinator.com