You have to fight to produce quality and you have to do it continuously. You cannot just merge in "working(tm) but not using best practices" and then turn on the quality-switch months or years later.
Your attitude is creating a culture where nobody cares and it's why many good engineers end up hating their job.
Wow. You sure are extrapolating a lot about me from a simple comment.
While we're extrapolating, are you the type that makes junior developers rewrite their loops as list comprehensions or the type that makes them rewrite their list comprehensions as loops? Because I've seen both in code reviews.
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.
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.
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.
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.
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.
Your attitude is creating a culture where nobody cares and it's why many good engineers end up hating their job.