
Code review review is the manager’s job - kiyanwang
https://hecate.co/blog/code-review-review-is-the-managers-job
======
barrkel
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.

~~~
telltruth
If your team has pure “people manager”, run!

~~~
jon-wood
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.

------
ndh2
The author tried to be clever with the title, but failed to take into account
how human perception works. The second "review" gets filtered out like that
gorilla in the basketball video.

~~~
techopoly
Oh my gosh...I didn't even realize the word repeats...

~~~
andyidsinga
holy shit - got me too!

------
ahartmetz
Yeah, I see more projects with bad code review culture than good. There are
many things that managers can do about it.

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

~~~
quantumhobbit
I just left a job with a truly toxic code review culture.

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

~~~
BeetleB
>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.

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

------
EliRivers
Before jumping in with your thoughts on how managers should or should not be
involved in code review, note that this is about code review review. Reviewing
the code review process, and reviewing the code reviews.

~~~
quii
Hoping there will be a thought leader who will show us how to conduct code
review, review, review.

~~~
dbcurtis
His name was Andy Grove.

------
DanielBMarkham
I agree with this in principle. Gets a lot tougher in practice, though.

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

~~~
pavel_lishin
> _Management should be a skill, just like all the other dozens of skills tech
> teams manage in various ad-hoc ways._

I would phrase it as "like all the other dozens of skills tech teams half-ass
in various broken ways".

~~~
DanielBMarkham
I think an important point in modern technology development is that to make
anything of value happen, you constantly deal with dozens of various silos,
each of which can be a lifelong specialty area. You have to learn to skim most
of the time and be able to dive deep -- a bit -- as necessary.

We may be saying the same thing, but I like my way better :)

~~~
pavel_lishin
I agree that everyone needs some management skills, but I'm not convinced that
most teams can be run without a dedicated manager.

~~~
DanielBMarkham
Agreed. The same as most teams cannot be run without a dedicated DBA. The
question is whether or not the DBA does other things, not whether that skill
is needed or not.

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.

~~~
mmt
> 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

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.

~~~
DanielBMarkham
I apologize. I believe you may have misunderstood brevity for cynicism.These
are systemic problems. There are no bad actors and everybody means well.

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.

~~~
mmt
> I apologize. I believe you may have misunderstood brevity for cynicism.These
> are systemic problems. There are no bad actors and everybody means well.

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.

------
crdoconnor
>Code review, whether via a pull request or other means, still is one of the
best bang for buck methods for finding defects in code.

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.

~~~
johnb
Author here. The post is largely written from a point of personal experience
and philosophy but that's the one statement I'm comfortable saying has
empirical backing.

I link to [https://blog.codinghorror.com/code-reviews-just-do-
it/](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.

~~~
JackMorgan
As a firm believer in TDD and pair programming, I would caution you that since
ultimate goal of programming productivity is unmeasurable, defect rate could
also just be a measure of how much slower those practices are.

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.

------
maxxxxx
How do these benefits compare to what you get from pair programming? If the
pair works well together I think pairing provides more learning and better
quality.

~~~
Cthulhu_
Code review is (from my point of view) a way to have a second or third person
look at the code from a distance, in his own time, with less context than the
author. When I write code I'm far into it and stop seeing the bigger picture.
When you pair program, I think the same thing happens. Finally there's the end
product, which can be looked at from a distance by a reviewer.

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.

~~~
maxxxxx
How much time do you take for a code review? If I did a full review that
really tries to understand the code it would take me close to the time it took
to write the code. But I don't get that time. So I pretty much nitpick on some
style issues but rarely find any real problems.

~~~
bluesnowmonkey
That's just it, code review done right takes time. If the author spent four
hours on a change, I might spend an hour just reviewing it. You have to
understand the problem and the solution just as well as the author.

------
sgt101
Are there any field studies that demonstrate the impact and efficiency of
these practices? Is there a body of evidence to support the assertions that
are made in this post?

~~~
johnb
Hi - author here.

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/](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](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.

~~~
sgt101
Some good points - many thanks.

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.

~~~
johnb
I agree and trying to cover that education gap is what I'm trying to achieve
with the post (and the product behind it)

------
bradenb
This is a really interesting read. Thanks. I have found in my own experience
that developers tend to include the same people over and over again in their
PRs and what ends up happening is people tend to just "trust" what the
submitter has committed. This often leads to PRs that are almost blindly
approved. By involving management to keep the PRs as a tool for discussion
(social and technical), I think this could be avoided. It's probably also a
good idea to mix up the reviewers (a healthy mix of people familiar with the
code and those not familiar with it).

------
dave_sid
But is the code review review review the manager’s manager’s job?

~~~
pvarangot
Of course it is it is

------
atomicbeanie
-1 Moving the evolution of a groups code into the hands of the person who works with it the least is an anti-pattern. Driving change from within a team can be facilitated by a manager who reads the group's code, but any manager who thinks they are going to be the judge of what the team is achieving is deluded and overly enthusiastic about their authority.

------
johnb
Post author here - logging off to go to bed because it's bedtime down under.
Will pop by again in the morning.

Thanks for reading and all your comments.

------
sorval
A manager sitting in meetings all day will be poor at responding to review
requests. Its the same reason they make crappy oncalls.

------
mychael
Please ignore the advice in that article. It's hinged on the false idea that a
manager always knows best.

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.

~~~
hyperpape
How is an un-self-aware manager ever going to be effective at anything?

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

~~~
mychael
> meta review

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?

~~~
AlexCoventry
I've seen pull request interactions where a moderating authority would have
made all the difference.

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.

------
sonnyblarney
This is a very good article.

------
wavefunction
On my team we're all managers. We manage ourselves and our output, and we
manage our team, team's output and each other by supporting and assisting each
other. So from that standpoint I agree. I don't see the need for dedicated
managers though I do value our dedicated scrum/agile coach though, who
encourages all of that.

------
0x8BADF00D
This is a terrible idea because it will lead to a more hierarchical corporate
environment. My manager has been coding for 15 years in the embedded space,
but he is rarely on my PRs (unless we happen to be working on an overlapping
feature).

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.

------
agounaris
No its not, code review is the team's job, including the manager.

~~~
EliRivers
It says code review review, not code review.

------
something2332
This is fine if the manager is also shipping code. If the manager is not
shipping code then it's really not cool to do code review. You won't earn
respect that way.

