11 ms·

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 junio

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.

sidlls 4y ago
"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.
- Ensorceled 4y ago
  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.
  - lelandfe 4y ago
    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...?
    - astrange 4y ago
      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.
      - dilyevsky 4y ago
        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.
  - penjelly 4y ago
    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.
  - fatjokes 4y ago
    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.
  - sidlls 4y ago
    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.
    - Ensorceled 4y ago
      > 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.
      - deleted 4y ago
        [deleted]
      - sidlls 4y ago
        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.
  - strix_varius 4y ago
    > 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.
  - dilyevsky 4y ago
    > 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
bt4u 4y ago
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.
- ferdowsi 4y ago
  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.
- Ensorceled 4y ago
  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.
  - mjlawson 4y ago
    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.
    - Ensorceled 4y ago
      > 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.
ferdowsi 4y ago
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.
peteradio 4y ago
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.
- Ensorceled 4y ago
  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.
  - schrodinger 4y ago
    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).
    - nonethewiser 4y ago
      > 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
    - deleted 4y ago
      [deleted]
kissgyorgy 4y ago
People without technical background shouldn't be allowed to be managers of highly technical teams.
- Ensorceled 4y ago
  I agree. Wait, do you think I'm non-technical?
  - razemio 4y ago
    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.
    - Ensorceled 4y ago
      > 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?
      - strix_varius 4y ago
        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.
baby 4y ago
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.
- bigyikes 4y ago
  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)
rmk 4y ago
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).
justahuman74 4y ago
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'
- Ensorceled 4y ago
  Yes. Why do you think I have code reviews if not to stop messes from getting made? I'm not talking about that.
- prettyStandard 4y ago
  > reasonably well written, no major flaws and they forced the junior to rewrite because it wasn't "best practices"
doutunlined 4y ago
"It works" is perhaps the lowest bar you can possibly have for merging code.
- beirut_bootleg 4y ago
  "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...
BlackjackCF 4y ago
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.
- makeitdouble 4y ago
  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.
azov 4y ago
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.
- Ensorceled 4y ago
  > 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.
- projectazorian 4y ago
  > "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.
  - klardotsh 4y ago
    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.
makeitdouble 4y ago
> 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
- Ensorceled 4y ago
  > 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"
  - projectazorian 4y ago
    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
    - geraldwhen 4y ago
      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.
  - logbiscuitswave 4y ago
    “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.
_dain_ 4y ago
galling. gauling is what the french do.
- Ensorceled 4y ago
  Thanks ... clearly to much Astérix & Obélix.
moomoo11 4y ago
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.
femto113 4y ago
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.
- nonethewiser 4y ago
  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.
fzeroracer 4y ago
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.
rubyist5eva 4y ago
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.

sidlls 4y ago

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

Ensorceled 4y ago
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.
- lelandfe 4y ago
  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...?
  - astrange 4y ago
    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.
    - dilyevsky 4y ago
      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.
- penjelly 4y ago
  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.
- fatjokes 4y ago
  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.
- sidlls 4y ago
  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.
  - Ensorceled 4y ago
    > 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.
    - deleted 4y ago
      [deleted]
    - sidlls 4y ago
      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.
- strix_varius 4y ago
  > 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.
- dilyevsky 4y ago
  > 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

bt4u 4y ago

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.

ferdowsi 4y ago
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.
Ensorceled 4y ago
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.
- mjlawson 4y ago
  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.
  - Ensorceled 4y ago
    > 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.

ferdowsi 4y ago

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.

peteradio 4y ago

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.

Ensorceled 4y ago
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.
- schrodinger 4y ago
  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).
  - nonethewiser 4y ago
    > 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
  - deleted 4y ago
    [deleted]

kissgyorgy 4y ago

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

Ensorceled 4y ago
I agree. Wait, do you think I'm non-technical?
- razemio 4y ago
  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.
  - Ensorceled 4y ago
    > 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?
    - strix_varius 4y ago
      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.

baby 4y ago

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.

bigyikes 4y ago
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)

rmk 4y ago

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

justahuman74 4y ago

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'

Ensorceled 4y ago
Yes. Why do you think I have code reviews if not to stop messes from getting made? I'm not talking about that.
prettyStandard 4y ago
> reasonably well written, no major flaws and they forced the junior to rewrite because it wasn't "best practices"

doutunlined 4y ago

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

beirut_bootleg 4y ago
"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...

BlackjackCF 4y ago

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.

makeitdouble 4y ago
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.

azov 4y ago

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.

Ensorceled 4y ago
> 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.
projectazorian 4y ago
> "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.
- klardotsh 4y ago
  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.

makeitdouble 4y ago

> 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

Ensorceled 4y ago
> 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"
- projectazorian 4y ago
  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
  - geraldwhen 4y ago
    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.
- logbiscuitswave 4y ago
  “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.

_dain_ 4y ago

galling. gauling is what the french do.

Ensorceled 4y ago
Thanks ... clearly to much Astérix & Obélix.

moomoo11 4y ago

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.

femto113 4y ago

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.

nonethewiser 4y ago
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.

fzeroracer 4y ago

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.

rubyist5eva 4y ago

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.