Code review review is the manager’s job
hecate.co
hecate.co
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.
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.
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.
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.
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'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.
* 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.
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.
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.
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."
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
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.
* Assign enough time for code reviews - otherwise developers will just care about their own code submissions and let reviews rot.
* Establish productive code review rules. E.g. always go from big picture to details, otherwise you'll waste time on details that you end up throwing away.
* Make reviewers co-responsible for the quality of code reviewed, otherwise instant acceptance for everything becomes attractive.
I have a recollection of seeing a study once that found the best balance point of review time vs dev time but haven't found it again. I recall it being somewhere around 20-30% but now that I can't find it I'm questioning my recall.
So assuming the same payoff benefit (big if, but let's go with it) between PR style reviewing and more formal inspections, every engineer should be spending around four or five hours a week performing reviews to hit the sweet spot.
- managers would swoop in and leave incorrect criticisms. Asserting plainly false things and in one case reviewing python code as though it was JavaScript.
- devs would make conflicting requirements for improvement. Dev A demands change A. Dev B wants change B. A and B are incompatible and neither dev will back down. Management of course refused to help settle the argument.
- change requests completely unrelated to the PR being reviewed would get crammed into the PR. So every small code change could balloon into weeks on refactoring work.
- Often times code review comments were made without ever reading the code being reviewed. Such as a comment of “Why aren’t you using library X?” sandwiched in between invocations of library X.
The culture was clearly rewarding people for shitting on others work instead of doing any work themselves. Getting even the simplest code changes past required endless meetings explaining the most basic facts about computers.
So glad to be gone from there.
This was a problem in my previous job. Options I thought of:
1. Get a moderator. This is what we did. He did not just settle disputes - he did more than that. But it's good to have at least this role.
2. Have a total of 4 reviewers review the disputed section. If there is a majority, change it. If it's a 2-2 split, just leave it as is. (Team did not accept this).
3. Disputed code (whether changed or not) should have comments saying this vs the alternative was discussed in a code review. Why? Because if there isn't consensus, over time, people are going to keep changing those lines of code back and forth (some developer who was not involved in the review will decide to refactor - rinse and repeat).
Even with that structure I found it challenging to develop a good reviewing style that picked up the important stuff without getting bogged down in silly details that lead to arguments. Reading John Ousterhout's "A Philosophy of Software Design" was really helpful. He focuses on module decomposition, quality of public interfaces, and documentation among other things.
So I won't submit to a committee review before my code is accepted. Its a recipe for anguish and failure.
Module decomposition is usually something I try to pick off in design, but when accepting pull requests from outside it's a critical consideration. We're not blocking creativity but trying to ensure that the code base not only remains high quality and that the design morphs appropriately over time as we meet new requirements. By making this a review point you can help create a culture where engineers think about it before submitting. That in turn makes it easier to justify dates to managers.
Well, that's a nice rule. Another one I heard people say is that code reviews are not for arguing about architecture or design decisions. If we take those two rules together, code reviews are good neither for discussing the details nor for discussing the high-level picture. Which in my experience is about right.
Overly-complicated code is a mistake. Maintainability matters almost as much as a passing unit test. You want your code reviews to catch meaningful things, but they often end up being about things like this (fixing inefficient small bits of code).
e.g.
function isDry(weather) {
if (typesUtil.isSunny(weather) ||
(!typesUtil.isSunny(weather) && typeof weather.hasSnow === 'boolean' && !weather.hasSnow)) {
return true;
}
return false;
}
function isDry(weather) {
return typesUtil.isSunny(weather) ||
(typeof weather.hasSnow == ‘boolean’ && !weather.hasSnow);
}
function isDry(weather)
var hasSnow = false;
if(typeof weather.hasSnow === 'boolean') {
hasSnow = weather.hasSnow;
}
return (typesUtil.isSunny(weather) || !hasSnow);
}There is also "minor" stuff a linter won't catch, such as bad naming. IMO naming is actually quite important.
In the first instance, I suspect there is blame culture or otherwise the developers previously had that experience and thus carried the baggage to the new job. I believe this is how you can get pull requests that either never get approved or merged (nobody wants their name attached to it in case they get called out). When it comes to not having time to review at all, I suspect that's the classic "I'm too busy" excuse, where if the whole team is too busy working independently then they can't ever expect to get anything to production without a serious effort each time.
In that instance the code review and lack of productivity is just a symptom of a lack of team dynamic.
The latter point is weird because I've had that and I didn't enjoy it at all. I could never tell if the approval was because the work was good or because it was a rubber stamp operation. Especially based on other feedback it felt like I was being put on a pedestal and I hated that I could basically do no wrong, even when I did because we merged and then I had a gut feeling that I fucked something up.
I'm not sure accountability would fix that particular manifestation of the problem.
The best management I've seen was tapped into the pulse of the team and knew how people interacted and the problems we were struggling with. Keeping an ear to code reviews is no different than observing how your team interacts in person or in emails.
"Manager" really shouldn't be a job in tech, at least when it comes to individual teams. Management should be a skill, just like all the other dozens of skills tech teams manage in various ad-hoc ways.
Groups of teams plays out differently, since you're really starting to work at the meta level. Even then, though, management is just one of a bunch of skills necessary. (At that point, however, it may be so much of a primary skill to warrant a separate role)
I would phrase it as "like all the other dozens of skills tech teams half-ass in various broken ways".
We may be saying the same thing, but I like my way better :)
If you've got six or seven people that need constant management, you're working at a burger joint, not a tech shop.
If I remember correctly, in the book "In the Plex", Google scaled out to thousands of developers without having a dedicated management slot. (This is a good time to mention that all generalizations are false, including this one)
In general, in the tech world I find the biggest supporters of "X should be its own job" are either the people who do X -- or train people to do X. Customers don't care one way or another. Neither should we. If we need an X, we get one.
I don't imagine it as someone whose job it is to ask me once an hour, "so how's it going, can you code any faster?"
The good managers I've seen don't talk a lot. The best managers are developers who have a natural skill for the work and got voted into the job, They do that obstacle-clearing and coordination on an as-needed basis, working alongside everyone else the rest of the time.
The core of the problem is that tech development involves way more skills than there are people, no matter how you want to slice them. When you go for training or start putting folks in roles, the assumption is that your role is at the center of the org. Not the value creation stream. There's an oversimplification and false focus that occurs that's antithetical to getting things done. So yes, that's the manager's job. But it rarely stays like that.
That strikes me as a characterization with more cynicism implied than necessary.
I, personally, routinely support job specialization, even outside my own specialty. Partly, this is bias/solidarity, but, partly, it's that, even though I'm happy doing Y, I don't want to end up spending 20% of my time doing X (and neither does almost anyone else, which they may not admit).
> Customers don't care one way or another. Neither should we. If we need an X, we get one.
Except that, no, "we" don't get one, not if we've trained ourselves, in the entire industry, to think X isn't its own job. Instead, we get a Z-that-can-also-do-some-X (but may, as I alluded to above, actually not want to do that X, or, as other comments in the thread suggest, can't do all of X).
This is especially noticeable among startup job postings over the last 5 years where Z is programmer and X is either manager or ops (including the sub-specialty of DBA).
How do someone even know they need an X if no X exists in their world? How we talk about things, especially to newer entrants into the industry matters. We should care.
I came to HN because it was supposed to be a place to learn about how startups work.
I'm still here years later. One of the things I learned is that you let the market pull your startup. You can't push it. In other words, you don't know when trying to make something people want where this is going to lead you. Instead you interact with the folks you're trying to help and that interaction drives the things you do.
There is much wisdom here.
The other major thing I've learned in this area, from my own work with tech groups instead of HN, is that people like doing what people like doing. That is, given their druthers, people will always drift back to doing certain types of things. Like making reports? I'll bet you we can take you out of this job, put you somewhere else in a completely new surrounding -- and in a few years you'll be doing reports. It's what you like doing.
We tech folks are really smart people, so we've managed to take everything we've touched, split it into smaller pieces, and make them complicated. It's what we like doing. Over many years, this means that any significant tech effort contains dozens of skillsets, all of which can be very complex. This number continues increasing.
So with limited resources, as a small org or startup, you're faced with either letting the job drive out what skills you have to learn/use -- or not getting the work done. In larger companies, you can have the illusion that somehow you just need to grow enough experts. Small efforts can't do this.
>>How do someone even know they need an X if no X exists in their world?
Because X is a skill, not a role. If you transition from "jobs require these roles" to "jobs require these skills", and you let the problem drive your learning and adaptation, it all works out. If not? Overhead and friction increase as the required number of people increase, leading quickly to very slow projects. Everybody gets a role where they do what they want, but it's at the cost of focusing on the value instead of the employees.
We naturally tend to view tech development backwards from what it actually is. So good people, doing their best to help folks, end up creating interwoven problematic nomenclatures, policies, and systems. Hopefully that sounds less cynical.
I, too, apologize, as I didn't mean to suggest your were implying malice. I meant cynicism in its fairly strict/traditional sense of presuming action based on (primarily or exclusively) self-interest. Being self-interested doesn't automatically mean someone is a bad actor.
Thinking further, I would object even to the brevity, since it appears to dismiss (or oversimplify) the supporters' motivations.
> The other major thing I've learned in this area, from my own work with tech groups instead of HN, is that people like doing what people like doing. That is, given their druthers, people will always drift back to doing certain types of things.
I absolutely agree, and I also think that, more importantly, the converse applies, hence my remark about people not wanting to do X.
> we've managed to take everything we've touched, split it into smaller pieces, and make them complicated. It's what we like doing. Over many years, this means that any significant tech effort contains dozens of skillsets, all of which can be very complex. This number continues increasing.
I disagree. There seems to be little evidence of it. We may have constantly shifting sub-specialties based on currently popular tools, but, for example, programmers are still programmers.
I don't believe there was ever a time of Renaissance Man tech folks, where the same person could build a computer from silicon and then program it equally competently.
> Because X is a skill, not a role. If you transition from "jobs require these roles" to "jobs require these skills", and you let the problem drive your learning and adaptation, it all works out.
I don't get it, I'm afraid. Is there more to "skill not a role" than semantics? Even the statement "jobs require these roles" sounds awkward because its just too many words. The job is the role, so no transition needed. I'm pretty sure it's already understood that the job/role requires those skills, but, more importantly, is primarily (or even exclusively) about those skills. For people who want to do X (as well as for those who want to avoid X, even if they possess some of the skill), that can be important.
You mean like how system administration has been downgraded to a skill that is tacked onto developers via DevOps?
I don't find that to be true. It's a good way of keeping code quality in check, which has an indirectly positive effect on catching defects (as well as a number of other positive effects), but it's not a good way of catching defects itself.
I link to https://blog.codinghorror.com/code-reviews-just-do-it/ in the post which has an excerpt from code complete:
> … software testing alone has limited effectiveness – the average defect detection rate is only 25 percent for unit testing, 35 percent for function testing, and 45 percent for integration testing. In contrast, the average effectiveness of design and code inspections are 55 and 60 percent.
I don't think that's the case, but really we have no way to prove that it's true. Just a word of caution if you are ever trying to convince someone, it's best to be up front with the possible flaws of those studies.
I'm often thrown in to a project with bad to mediocre design and I am not able to just change the design to become "good" just by reviewing PRs. I can only attempt to push the design into a slightly better direction over a long period of time.
Similarly, just because it's reviewed doesn't mean that the design is great.
I am probably able to hit something approaching 45% defect detection rate with integration testing on most projects though.
With code reviews I'd say it's about 1 or 2%. Bug-wise, I usually only spot fairly obvious "language" gotchas (e.g. initializing an empty list in a method signature in python).
My anecdotal experience with my own teams is relying too much on pairing as a training mechanism, especially between senior and junior, has a couple of issues. The first is tradeoff between training quality and utilisation - it may shorten the training cycle but the payoff isn't high enough for the output constraint on the more senior member. The second is I've sometimes had trouble weaning the junior off always having guidance on tap and it takes them longer to develop confidence to break through certain problems alone.
While yes, pair programming is preferred over just pushing to master, I think code reviews (and moreover formal code reviews) are better than pair programming for the final OK.
If you're reviewing someone's code at the code review stage, you can only do so much. If it's working, tested code then asking for major changes is either pointless or very costly; there's little point in suggesting out a better way of approaching a problem if it requires major surgery. Unless you spot a potential bug or something that will cause major problems, you're realistically restricted to suggesting minor changes like style suggestions or better names for things.
With pair programming you have two people collaborating on a design from the start where suggestions for better ways of doing things are much easier to incorporate. If pair programming for a complete feature is either not feasible or undesirable for some reason it is at least often worth pairing to start a feature off.
Code review on the other hand also has some benefit in getting a fresh pair eyes on a piece of code to spot obvious mistakes that people elbows in deep in it have glossed over or find hard to see, such as left-in debugging code or user credentials.
The quality assertions are the most defensible empirically. I wrote the post at the office away from my bookshelf which was the main thing keeping me from including them up front (flicking through my old copy of code complete now). Capers Jones' "Software Defect-Removal Efficiency" (https://ieeexplore.ieee.org/document/488361/) would be the main one on the effectiveness rate of code inspections. I've collected a few other relevant papers over at https://github.com/joho/awesome-code-review
The second two themes around education and culture come more from my personal perspective and experience - largely from running the engineering teams at Envato and 99designs respectively. My feeling is the educational and safety elements of code review have come up online a lot more over the past couple of years - but that's just my anecdotal view.
I think that there is a market failure / exec education issue around these point though. The impact of the improvements measured/evidenced is in the maintenance cost of the delivered artefact and the improvement of the process for future delivery. These are "over the fence" issues for the owners of the delivery shop. We need to get it into the bosses minds that they are paying for things that will be either good in the long term or expensive in the long term - I think that there is a lot of hiding and hedging and shifting of burden (to users) that goes on around that.
Pair programming vs code review, Nodejs vs Ruby, React vs Vue, Vim vs Atom, etc etc etc are all 100% emotional and subjective decisions. Therefore they are subject to fashion, being cool, and herd mentality.
Accepting this is an essential step on the path to become a well-rounded engineer.
I agree that the bulk of X vs Y comparisons are emotive and subject to what Alan Kay calls the "pop culture" but that in no way demonstrates that measurement is out of reach. It's just in most contexts we don't care to do the work to measure it out of personal preference or economic reality.
I really like this post and the one about managers programming btw. Your common sense explanation to why most new managers end up coming back and reporting that they shouldn't code is exactly what I've seen as well.
In it he has a pretty interesting insight about late projects. "If a project offered a value of 10 times its estimated cost, no one would care if the actual cost to get it done were double the estimate. On the other hand, if expected value were only 10 percent greater than expected cost, lateness would be a disaster."
I imagine as our field matures measuring productivity and hitting estimates will get more important once software has finished eating the world. The unlimited (or close to) upside dev projects will be largely behind us and we'll need to hit tighter economic targets. I'm just glad I'm working in the era where I don't need to measure it :D
Glad you like the posts. Thank you!
It is sad in a way, but also exciting as you say to be in the golden age.
Thanks for reading and all your comments.
Imagine for a second that the manager is wrong in his/her judgement. Now the developer has to choose between arguing with the manager and risk being viewed as insubordinate or just going along with the feedback and letting the software suffer the consequences.
The author notes your objection, and addresses it:
"There’s a fine line to be walked between managing the pull request process and micro-managing it. What happens within any specific pull request is the domain of the team so you should resist the urge to intervene frequently within the process itself. The behavioural trends in the process are your domain and if you’re running a meta-review process well your work will be predominantly behind the scenes."
Is "meta review" really a value add for software companies? Is this advice for Managers of peace-time companies who can't find anything more valuable to do with their time?
We mostly accept the need these days to moderate other online social behavior, like on HN, to reduce potential damage from ego/politics. Hopefully the need is lower in a professional environment like SD, but I could still see it being useful when things go off the rails.
And yes, effective culture around PRs seems valuable. My current team seems to have very inconsistent expectations, and would benefit from someone with authority steering people in the right direction.
No it's not. It's hinged on the idea that the manager's job is to teach his people with the knowledge and experience he's acquired. None of this suggests he's infallible, nor does the article say as much at any point.
> Now the developer has to choose between arguing with the manager and risk being viewed as insubordinate or just going along with the feedback and letting the software suffer the consequences.
If the culture is such a rigid hierarchy, they're not going to be able to deal with a senior engineer any better.
And when you have subordinates this cultural problem is easy to fix. They'll invariably think you're wrong, and if they're reluctant to say so, you just tell them, "if you think I'm wrong, it's your job to say so, so out with it."
At my company, PRs will automerge if at least one reviewer with write access approves and CI checks pass. This is the best way IMO. It’s fast, too.