The manager can do code review if they're a player-manager. If they're a people manager, it's a lot harder to meaningfully review. Managers usually hired in to manage after the team grows (and splits), rather than trying to convert developers into managers, so they may not have a good idea about the code at all.
I'm more inclined to think that it's senior technical responsibility to review, since they'll have a lot more knowledge of the codebase, its evolution and future direction, potential gotchas and various cultural dos and don'ts.
For sure, it's important to keep an eye on the code review culture, though. It's easy for code review to degenerate into nitpicking small things like whitespace, naming, coding conventions - these should be handled by tools instead. And of course if people are being assholes, they need to be pulled up on it.
The double "review" in the title is not an error. The manager's job isn't to do the code reviews. Their job is to review the code reviews and make sure that they are being done well and that review feedback is being given constructively.
You're right about the double review, but I think the argument still loosely applies even removed one step.
For instance, I've worked somewhere with a rule that x% of code should be unit tested. In fact that was the rule applied in code reviews, anything else was a free for all between projects including any other form of testing coverage. And the rule was applied regardless of the size of the project, whether it was UI code, an I/O library or a few lines of tcl running on an F5.
If I was running a project and we didn't make the mark for unit testing it was usually something I could justify to a player (or ex-player) manager. A couple of others would call a full stop to development, pull everyone they could into a meeting and try and berate us for being substandard. Eventually someone with a technical background further up the chain would calm them down until it happened again.
I guess that's partly a function of them being poor managers and I'm sure some less technical mangers know where the line is, but I've come to consider anything about the tools and process as the responsibility of the technical lead and their team and outside the remit of management. If what the team produce is substandard they need to fire everyone, have someone technical look at who to fire first, or some smaller tweak, rather than have people who've never done the job make a judgement on the efficacy of the approach.
Of course, there isn't a binary divide between technical and non-technical and all projects are different so YMMV.
My first reaction based on the title was probably the same as a bunch of people here, but then I reread the title. I agree, it absolutely is the manager's job to make sure the process is positive.
In a lot of cases, I see variations of this when there was not enough up front thought put into the design (before writing code).
Slight tangent, but a good way to tackle that is to outline what you want to do (sketch the API you're going to fill in, etc), and run that by a reviewer
That's not a code review. That's either a suggestion that the initial commit is 'wrong' or the reviewer is a jerk. Either way, this is the PM's fault - not enough information to finish it right or too reviewer nitpicky on useless stuff.
Money quote: "Nothing should be surprising to a reviewer in a code review: discuss any significant design decisions before code is written. Code reviews are a time to iron out implementation details, not discuss major design or architecture decisions."
What if you use pull requests to prototype architectural change? My take is your pull request should include the context of the change and the sort of feedback you want.
It was pretty common at my last job; seemed like every third PR got at least one request for a total or near-total rewrite, usually for completely non-functional changes without obvious benefits over the solution implemented.
I've been asked to make my solution more complicated and to add usage of things like the friend keyword to unit tests instead of testing a public static function.
> For sure, it's important to keep an eye on the code review culture, though. It's easy for code review to degenerate into nitpicking small things like whitespace, naming, coding conventions - these should be handled by tools instead.
I agree with this, but think there needs to be some balance. My experience has been that people generally add rules over time while rarely removing them - usually to quell disagreements without regards to the benefit of standardising the answer versus the cost of more rules.
a place where I've worked the people that make all the strategic decisions about what to work on don't/can't work on the code base.
The steady stream of senior developers that exit the company suggest that this strategy isn't the best one. I think player-managers should arguably become the norm.
"Architect who doesn't code" is a terrible thing. I've designed projects and then turned them over to other skilled developers more than once, while I help them out with reviews and such. My knowledge is noticeably rusty in under a month and by month three I'm easily wrong at least as often as I'm right about what some other bit of code is doing. To even help them out on occasion requires a bit of re-orientation; I can't imagine trying to hand them fresh architecture requirements.
In fact, I've noticed it's kind of concerning when this doesn't happen; it means the engineers who took it over lack the confidence for some reason to make non-trivial changes to the code base and are just pecking at the sides. Unless the project is deliberately in maintenance mode, that's a problem. Could be people, could be me, could be them, could be technical issues, could be lots of things, but there's certainly some sort of problem there.
Recent experience has taught me to think of those type of people as delivery managers, rather than people managers, and so long as they understand the limits of their knowledge they can be invaluable.
I've worked with two people who were excellent in that role, they work with the rest of the business to clearly define priorities, and to ensure they understand what we can and can't deliver. When it comes to hard technical decisions they delegate to technical leads and developers, but they have enough experience of engineering to be able to intuit whether what they're hearing makes sense.
I've also worked with someone in the same position, but without the self awareness to know where his knowledge ended and it was time to delegate. That was possibly the worst six months of work I've done, but thankfully he was eventually forced out by the board.
One of the best managers I ever worked for was a “people manager”. He knew his limitations and worked within them. He was reliant on the tech lead for technical knowledge but between the two of them made a pretty good team.
Some of the worst managers I’ve worked for have been technical people who don’t really want to manage but git Peter Pricipled into management.
Not a pure people manager - managers with a background of coding are important so they won't swallow BS, can understand tradeoffs between e.g. refactoring vs accepting tech debt to get a feature finished quickly, etc., understand the reliability or otherwise of dev estimates, etc.
But few strong developers are also strong people managers. The two attributes seldom overlap. And I think being good with people is more important in a manager than being good with code.
Of the companies I've worked at, my favorite setup was:
* people manager dealt with people issues
* technical reviews performed by senior technical people (under the same people manager, or cross teams if noone else is at the same / higher level)
* technical review is a part of the people manager's management responsibility: is it done on time, is the reviewee satisfied with the content and criticism of the review, etc.
This prevented a lot of personal opinion blight from influencing technical reviews- aka avoided subjective criticism from being part of the review score, focused instead on more objective metrics.
The time and place for subjective criticism and discussion is in planning and in-situ reviews (aka pull / merge requests), not formal reviews.
That's my experience at least; the particular managers I had at that company (3 over 7 years) also happened to be excellent people on the whole. A bad people manager is, of course, just as bad as a bad technical manager.
Management skills are orthogonal to technical skills.
People who are good technically are not necessarily good at management and should not be put in that position. They should however be put into a code-reviewing position.
Where I work, we separate the roles of Product Manager and Engineering Manager. The former focuses on roadmaps and priorities and works with the engineers directly while the latter focuses on people’s growth and career goals, and has almost no say on what people are working on or the handing out of tasks.
It works well. It’s nice to have someone to talk to and work with who isn’t invested on what you are working on today or riddled with conflicts of interest.
I'm more inclined to think that it's senior technical responsibility to review, since they'll have a lot more knowledge of the codebase, its evolution and future direction, potential gotchas and various cultural dos and don'ts.
For sure, it's important to keep an eye on the code review culture, though. It's easy for code review to degenerate into nitpicking small things like whitespace, naming, coding conventions - these should be handled by tools instead. And of course if people are being assholes, they need to be pulled up on it.