There are better places, and ways, to teach junior developers than long, painful code review processes.
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 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.
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.
Like what? And dont tell me it’s some kind of lecture/lab-style group meeting for 1-2hrs
Your attitude is creating a culture where nobody cares and it's why many good engineers end up hating their job.
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 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.
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.
I squashed that.
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)
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.
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.
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.
But that is a task
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.
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?
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.
> you assume the "senior developer" is ALWAYS in the right, and the junior and the "non-technical" manager...
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.
'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.
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.
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.
Holy shit. I should have just written this.
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)
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).
I'm not talking about that.
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.
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
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"
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
As a manager, the Junior shouldn’t be on the critical path solo. That’s just bad management.
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.
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.
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.
"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.
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.
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.
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.
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.
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.
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.
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.
You sound like an absolutely insufferable person to work under.
it's especially 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.