Relating to the authority and gatekeeping sections at the end, there’s a cousin of “late-breaking design review” where you’re not just a random developer who’s peeved about a certain style: you’re the person (or a member of the team) who has domain authority, who should have been consulted from the beginning, but who only found out that this project exists when you got tagged on a code review.
And lo and behold, your esteemed colleagues have written something that won’t scale, doesn’t have any observability, breaks every style rule in the book, and reinvents three libraries that the rest of the company has standardized on. And yes, they tagged you for review, but they really just want a checkmark, because there’s a deadline and they need to be in production in the next 37 minutes.
That happened to me a few times. What I did, and it worked reasonably well, was:
Coordinate with the person who wrote the patch and see if there’s anything that they can integrate right away, like, within the next 37 minute.
Open tickets for the other problems (drop the reinvented libraries, fix whatever major design flaw results in low performance or whatever). Usually multiple smaller tickets, since whoever pushed them to do this without poking me in the first place is unlikely to be receptive to the idea that it needs a massive revamp.
Document, or improve the visibility of the documentation of, the “proper” design. First, because if something is effectively a solved problem, design-wise, the solution (or at least general awareness that there is a canonical solution to it) should be accessible to anyone in the team. And second, to establish a chain of awareness that they can talk to me at the technical level, rather than rely on someone at the management layer figuring out that it’s the kind of problem where they should consult me.
Find a way to get whatever performance, observability or supply chain issue in the relevant test/audit trail (performance benchmarks, coverage figures, whatever), to ensure that #2 actually gets done. It’s usually not necessary but I found out the hard way that it’s a good precaution (engineering manager was completely clueless about both who knows what and about how much experience people in the team had, and closed the tickets at #2 so it didn’t look like he completely borked assignments).
This does of course depend on the person with the patch meaning well and just being stuck between a rock and a hard place. Lots of people just get caught on the wrong side of team dynamics, especially early in their career. If this was the best they could do under the circumstances, I’ll gladly help improve the circumstances for next time.
If they came up with that just because they couldn’t be bothered to look around the source tree for a bit, then I:
Point out every problem so nobody can say I didn’t do my job
Tag whoever was in charge and let this mess happen on their watch to ask if they want it merged nonetheless because it’s ugent
Revel in that sweet, sweet job security that the subsequent stream of bugs provides and which get assigned to me because, yep, I’m the domain authority. Oh and update my resume just in case that’s not an accident but the new company policy, because I also have my career to look after and I’m not going to get stuck cleaning up after people who don’t want to do their jobs.
Man, you’re nice. Last place I worked every code review for features that “needed to ship” was blocked for months usually resulting in teams being disbanded and projects cancelled over smaller code review issues than this
Yes! I’d love to know some solutions for this case! Maybe related are the reviews where I realize that the overall design that was chosen will cause problems in the future.
Some ideas that I resort to:
create new tickets for larger parts that are missing or broken (as long as they are not world-ending), and then concentrate my review on the other parts.
see if I have another ticket coming up to work on that project anyway; in that case I can fix the larger problems soon myself while working on that ticket.
Overall I’d prefer to prevent this situation, though. But I’m still not sure how.
From other companies I’ve heard of these approaches:
do a mandatory “design review” of every ticket before starting coding, where a senior dev reviews the proposed solution. In practice the design review is rarely done in practice.
do a “refinement meeting” where the technical aspects of all upcoming tickets are discussed. No idea if this works, though.
have a dev whose sole job it is to clean up code and design. Reportedly, this leads to quite good results; and it makes reviews easier because everyone knows that all “niceness issues” and most design issues will be solved in the future. It might be kinda expensive, though?
Love it, I have seen all these anti-patterns over the years, looking backwards, more often than not, the code review nitpickers were the worst engineers in the team.
Oh, yeah! I don’t think people realise just how counter-productive nitpicking is. Counter-productive as in what a net negative.
The worst I’ve been through was completely self-inflicted. The whole review process encouraged nitpicking. I was part of a team where code review happened by that dreadful “just set some time aside” rule. Code review wasn’t a real task that was assigned to someone. Instead, when a pull review was opened, the PM would just ask whoever seemed to have a shorter task queue at the time to review it, whether they were familiar with that module or not, and whether it was a five-line patch or a complete new feature. And, of course, being diligent in your code reviews was a big thing in the annual review, so you kind of had to say something.
First off, that obviously delayed them on the “actual” tasks they’d been assigned (as in the ones on the issue tracker), so they wanted to be done as quickly as possible. More importantly though, “an hour or two” was nowhere near enough to become familiar with the subject matter if you were reviewing a feature (I can’t be too explicit for obvious reasons but the volume of a “feature” was comparable to, say, writing a JSON parser or a minimal DNS server, so self-contained but possibly pretty big).
So most of the time you’d just get a long list of bullshit complaints like “frameCounter isn’t representative enough because it’s not obvious what kind of frames it counts”. Some people had pet peeves that weren’t in the coding standard but they insisted on (e.g. a single return statement per function). One person on the team gamed the system in the most insufferable way possible, he just ran the code through a linter and pasted just about every time, slightly reworded so it wouldn’t be obvious. It wasn’t the worst idea, except he ran it with a particular config (one of the first Google results for $linter config), so about half of them were either linter noise or just didn’t make any sense in context.
Trouble is you literally got so swamped fixing bullshit problems like these that nobody ever got to the real problems. After a few months I found myself literally altering the way I wrote my code depending on who I thought was going to review it just so they’d shut up about whether a loop counter should be called crtIndex because i is too short or i because crtIndex is too long and maybe look at important things I might miss.
Worse, because these bullshit review sessions routinely stretched over days and days, people would routinely address these “important” remarks on the run, while already working on something else. On at least three occasions I traced major bugs (as in crashes and/or issues with potential security implications) to typos or off-by-one errors from hastily-written “fixes” that someone had made one the fifth day of pseudo-reviews like these.
To this day I am convinced that that particular codebase would have been better off with no review at all than this charade. I can’t remember a single bug that was caught at review time, but I remember several that were introduced at review time.
The Great Silence: I mail you a patch. Nothing happens. No acceptance, no denial, no feedback. Maybe it got lost in the great void! Maybe it got silently spamfiltered. Maybe it just wasn’t to the developer’s liking. Do you send it again? Does that make it spam? Do you take the message and go away?
What I’m saying is, what happened to my Minesweeper quick-marking patch mail, Simon? ;)
I have had a few colleagues over the years that if they didn’t feel qualified to review your patch would just ignore it. Does not matter that they are the only people on the reviewer list. I would have to reach and ask if they were going to get to the review. “Oh, I don’t understand it”.
How it should be handled:
If you don’t understand it then likely something is unclear, ask questions. Documentation can be added either in the patch or separately.
Respond saying that you aren’t able to review it. Ideally recommend a better reviewer.
Both of these solutions don’t waste a week of waiting.
I’ve learned to appreciate The Great Silence. Asked in mailing list if a feature was welcome, they said it sounded reasonable, so I wrote the feature, tested it, sent the patches in. One person spent days arguing about the patches with me until it turned out he hadn’t even read my code. Another person called it a schizophrenic feature, and by extension called me a schizophrenic as well for having thought of it.
I’m a regular reviewer of a public open-source project (OCaml), doing reviews as a part-time / free-time activity, and I’m afraid I do most of these things on a regular basis. Oops!
The reason I can think of in most cases is “it turns out this way because I don’t have enough time to spend on the review, so the result is suboptimal but it’s a best-effort”, and sometimes it is “I tried to help by offering some review work for your PR, but in the end I asked more work of the author and the PR is not noticeably closer to being merged”.
More precisely, here is what I can think of as excuses and protections:
“the death of a thousand round-trips”: excuse: I don’t see how to avoid this when the reviewer has limited time available for review
“the ransom note”: defense: I try to have a sense of the scope of the PR, and not ask for things that are outside the scope. (“While reading your patch I noticed this thing two lines above that is really unclear, while you are at it could you refactor it as …”)
“the double team”: in my experience reviewers are not jerks and they try not to provide contradictory feedback; often there is a general sense that if reviewers disagree on something, the author can choose the option they prefer. I don’t see this as being an issue in practice.
“the guessing game”: I see more often something related but different, which is to say: “you did it using approach X, but I would rather prefer if you did it using Y”. The problem is that often the reviewer (myself) hasn’t really tried Y, so maybe it does not work, and it is hard to distinguish “I would rather prefer it for good reasons” from “I prefer it because it’s my idea, so I like it better”. I feel self-conscious when I do this, but often I believe that there are good reasons to prefer Y instead.
“the priority inversion”: excuse: seems difficult to avoid when there are several people doing reviews with different level of perspective
“the late-breaking design review”: excuse: same as above
“the catch-22”: my favorite approach for larger pieces of work is to have people implement a large PR that does the whole thing, submit it as a draft or just put it as a public branch somewhere, and then cut small, reviewable pieces out of it, submitted as individual PRs intended for merging. This gives readers a sense of the big picture (and it forces us to check that each individual piece is indeed needed and useful for the whole thing; often I work on a preliminatory refactoring that I think will be useful and in fact it wasn’t), yet let them review manageable chunks.
“the flip flop”: excuse: easy to avoid, but sometimes reviewers really think that previous code was going in a wrong direction (but the cost was not apparent then), and they ask to move to a more future-proof approach. I think that this is reasonable when the justification for the new approach is strong, not just taste.
There’s a huge difference between reviews in public FOSS projects and reviews in commercial projects. Many of these things are, to some degree, inevitable when the people who send the patches and the people who review them are independent developers working in their spare time, or at best under limited contracts.
But a lot of them are IMHO inexcusable in a commercial setting. Just two examples:
“the death of a thousand round-trips”: excuse: I don’t see how to avoid this when the reviewer has limited time available for review
Wherever I’ve seen that happening in a commercial setting, it was a symptom of the management team failing to either understand, or act on their understanding, that the time required to review a pull request is part of a task’s development time. A reviewer who has limited time available for review isn’t some inevitable artefact of the obscure craft of software development, they just have too much work to do for how much time they have.
“the double team”: in my experience reviewers are not jerks and they try not to provide contradictory feedback; often there is a general sense that if reviewers disagree on something, the author can choose the option they prefer. I don’t see this as being an issue in practice.
Many commercial settings have a specific sign-off process for this. If that process requires a sign-off from two reviewers, the author can’t choose the option they prefer :-).
There’s a second important factor at work in FOSS projects. They’re not entirely devoid of “office” politics and the like but most of the time the people working on them enjoy it to some degree. Whoever invested enough time in a project to end up regularly reviewing code is usually recognised by most of the community as being someone you can work with.
This isn’t necessarily the case in a commercial setting. Many of your points are based on you and other reviewers not being jerks. I’m sure you aren’t and that most people who work on OCaml aren’t, either, but working with complete jerks who are also designated reviewers is part and parcel of the corporate life.
The guessing game can be particularly frustrating. The reviewer has a solution in mind and only that solution will unlock the elusive green check. It doesn’t matter if the patch does solve the problem, it doesn’t solve it exactly how the reviewer has in mind and there’s no room for variation. I’ve seen this cause a lot of frustration for the submitter as they slowly make changes, getting closer to what the reviewer is looking for and wondering why the reviewer didn’t just submit the patch themselves.
One of the guidelines I try to follow is not leaving too many comments. If I’m that unsatisfied with the patch, it’s better for both of us to pair on the problem together.
There’s nothing worse than being “done” with a feature and then someone comes with 100s of complains.
Non-Blocking nits/ advice. Approve the PR but leave this if there’s just a minor like “yeah it’d be nice if we had some logging here but nbd”. I actually don’t want you to follow up on these in most cases, it’s not important enough but I’m leaving it up to you to decide how to spend your time.
Literal, actual bugs. Actual actual bugs.
Questions about serious, egregiously bad code that I won’t want to be on the hook for. I probably will approve with a “Can you tell me more about this/ I wonder if this could be simpler”.
If you block me on nits I will ignore you, resolve your comment, and wait for someone sensible to approve. You see a 1 line “cleanup”, I see 40 minutes of flaky test failures, CI/CD, context switching, branch switching, and also a 1 line “fix” for something that’s fine as-is.
By far my most common review comment is: “What you are doing here is tricky, and when I come back to the code in N months I will not be able to understand why this is done this way / what the subtlety is. Please write a comment in the source that explains what is going on.”. (Typically this is a matter of transferring explanations that are in the PR description itself into the code.) This not a hard-blocking request, but it is also more serious than the nits you describe, and I expect people to do it.
This is some of the most valuable review feedback. When I’m writing code, I typically have a load of surrounding context in my head. When I read code, I don’t. Someone telling me ‘I think I see how this works, because of this property from over there’ is useful and tells me I missed a very important comment.
I think there are probably some edge cases of “please do this”. Sometimes it’s warranted. I tend to err on not blocking depending on seniority/ experience - if this is a very new person with very little experience I may be less inclined to expect them to have the right context to make those sorts of calls.
If the code is difficult to understand, it should be improved (either by rewriting or commenting on the trickier bits) and this should be blocking.
If the code doesn’t follow existing conventions (I’m not talking stupid linter issues, but egregious naming differences, reinventing existing functions that are better reused from elsewhere etc), it should be blocked.
If the code is inefficient enough to make a noticable performance impact, it should be blocked.
Of course, I’m not arguing that the code has to be perfect before it’s merged. As a reviewer, you ought to be considering the long term health and maintainability of the code base, and not just the short term of “does this check the box of this feature?”. I think this is the main difference between a senior developer and a junior, TBH.
As a reviewer, you ought to be considering the long term health and maintainability of the code base, and not just the short term of “does this check the box of this feature?”
That’s a shared responsibility with the author. I’ve advocated for telling the developer these things, like “hey this code is confusing I think it’d benefit from ” or “this could be faster if Y”, but not blocking them on these things because it is a shared responsibility. I’ll only block in egregious cases where I just refuse to be on the hook for maintaining the code because it’s so bad, or literal bugs, etc.
If the code is difficult to understand,
This is a fine ask for, and I would ask for it, but if the code is tested and works and there are comments I’m not blocking on it. Also, if you can’t understand code, maybe you need to learn more idk.
If the code doesn’t follow existing conventions
Meh. Unless those conventions actually matter I don’t care and will never block on this, but I’ll bring it up and let them decide. Unless they’re going against conventions that will matter later, like maybe we have some custom Cache construct and it’s important to use it vs some other one.
If the code is inefficient enough to make a noticable performance impact
Doesn’t even make sense. All code is inefficient enough to impact performance. The question is whether the code meets requirements. If it meets the performance requirements the code is valid. If it doesn’t, that’s a bug, and I’ve said to block on bugs already.
TBH I think it’s a very “junior” attitude to think that code is the point. It’s not. The product is the point. Spending a ton of extra time to get the “code” better when you have a working product that meets requirements is just wasting time unless you have an actual product reason to do so (eg: “This will be costly to maintain” is a fine reason). There’s a balance, of course, and as I’ve said before it’s perfectly fine to say “this would be nice” or “I think this could be changed” etc etc etc.
hm, after re-reading your original comment in this light, I think we agree more than I thought. Initially it seemed to me that you have a reckless attitude about accepting anything that passes the tests and aggressively rejecting any improvement suggestions if they sound like too much work to you.
But if you see it as shared responsibility, that makes sense of course. Blocking then sounds more like “I won’t allow this code in at any cost”, essentially vetoing your decision as an author. In our company, we don’t have this kind of blocking / non-blocking distinction really. My team typically resolves all threads in a merge request before merging unless it’s explicitly stated that it’s not important. Not because we want to have the code “perfect” (whatever that means), but because we work on a system which is used in emergency situations, so we also have a very heavy QA pipeline and quite long release cycles. We want to make sure no issues crop up during QA or in prod. So robustness and maintainability is a very strong “product reason” for us, if you will.
I think I may have been overly brisk in my first comment. It’s the shared responsibility bit that matters. The author of a PR is often the one “closest” to their code, and I am closest to perhaps some pre-existing code. That means that I can give context about the code I have the most context on in order to help them understand how their code best fits into it.
But at the end of the day, they are a professional software developer just like I am. I am not interested in telling them how to do their job unless they’re doing something outright wrong. I will give them the context that I have (“this code is unclear to me”, “we usually do this here”, etc) but ultimately they’re an adult and I’m going to let them make the final call on whether they act on that unless they’re doing something that will really fuck things up or just makes zero sense.
If your product requirements are such that you must have very strictly guarded code, then that is perfectly fine. That means that when I see code that is not robust I will have to block that code because it is a “bug” if it does not meet that standard.
fn reticulate_splines(int n) -> int
if n == 42 { explode }
return 2*n
Does this function contain a literal actual bug? Assume that, at the specific Git SHA of the PR, there is no obvious situation where a caller of the function passes 42 for n.
I’m still hoping to work at a place where I finally get to enjoy the code reviews experience. So far I haven’t. Sometimes it does feel like there’s an element of “power play” in the review process.
Code reviews in private codebases are relatively new. How was the software that still runs the world written before them? They seem (in private codebases) to be the result of organizations no longer trusting their professional employees, testing on end-users, and cargo-culting a process suited to public projects - without evidence of benefit?
hm, I dunno. You could also ask how software got written before without version control, or without automated tests. People got by, but that doesn’t mean that’s the best way to develop software.
At my current company we use code review, and I like it. Not because I don’t trust my colleagues (although to be fair, I sometimes don’t trust myself!), but to increase shared understanding of code and increase its overall quality (yes, in a healthy team, bickering over what name or particular code idiom to use can actually be helpful) and also to prevent issues like inconsistencies, inefficiencies and the occasional actual bug. Often, the conversation on a merge request can also be useful when investigating problems.
At other companies I’ve worked for, code review was often seen as a costly thing to do - why spend the time and energy of two developers on one feature if you can have two developers working on two features? And then people wonder why things are unmaintainable when a developer leaves. Not doing any review (or pair programming, which can fulfill a similar role) also encourages fiefdoms, as each feature will have been built by a single person without much interaction with others.
hm, I dunno. You could also ask how software got written before without version control, or without automated tests. People got by, but that doesn’t mean that’s the best way to develop software.
Whenever anyone asks this kind of question, I am reminded of the STANTEC ZEBRA manual. The ZEBRA had a complex dataflow instruction set. To make it easier to program, it also had a ‘simple code’, which was a subset of common operations. Simple code came with a bunch of limitations, one of which was that programs could not be more than 300 instructions long. This is not a practical problem, the manual assures the reader, because no one could possibly write a correct program that complex.
The world’s moved on since then, but that’s usually the answer: they did it by dealing with far lower levels of complexity. A typical Excel spreadsheet does more complex calculations than anything that ran on the ZEBRA.
Yes, this. I worked as a programmer on payroll software on mainframes in the 1980s. A senior person told me almost exactly what to write AND provided tests. My job was limited to completing the translation of that into COBOL/JCL syntax, compiling, running the tests and double-checking everything.
In case you’ve wondering how the senior person got their work checked: there was one more senior person above them, but they were rarely involved. Mostly if the senior people found anything too difficult then it DIDN’T get done.
Relating to the authority and gatekeeping sections at the end, there’s a cousin of “late-breaking design review” where you’re not just a random developer who’s peeved about a certain style: you’re the person (or a member of the team) who has domain authority, who should have been consulted from the beginning, but who only found out that this project exists when you got tagged on a code review.
And lo and behold, your esteemed colleagues have written something that won’t scale, doesn’t have any observability, breaks every style rule in the book, and reinvents three libraries that the rest of the company has standardized on. And yes, they tagged you for review, but they really just want a checkmark, because there’s a deadline and they need to be in production in the next 37 minutes.
That happened to me a few times. What I did, and it worked reasonably well, was:
This does of course depend on the person with the patch meaning well and just being stuck between a rock and a hard place. Lots of people just get caught on the wrong side of team dynamics, especially early in their career. If this was the best they could do under the circumstances, I’ll gladly help improve the circumstances for next time.
If they came up with that just because they couldn’t be bothered to look around the source tree for a bit, then I:
Man, you’re nice. Last place I worked every code review for features that “needed to ship” was blocked for months usually resulting in teams being disbanded and projects cancelled over smaller code review issues than this
Is the “last place” still in business..? I worked at some place like that for a brief period, they ran out of money and shipped nothing.
Oh yes, they’re a major public company you’ve probably head of :)
Yes! I’d love to know some solutions for this case! Maybe related are the reviews where I realize that the overall design that was chosen will cause problems in the future.
Some ideas that I resort to:
Overall I’d prefer to prevent this situation, though. But I’m still not sure how.
From other companies I’ve heard of these approaches:
Love it, I have seen all these anti-patterns over the years, looking backwards, more often than not, the code review nitpickers were the worst engineers in the team.
Oh, yeah! I don’t think people realise just how counter-productive nitpicking is. Counter-productive as in what a net negative.
The worst I’ve been through was completely self-inflicted. The whole review process encouraged nitpicking. I was part of a team where code review happened by that dreadful “just set some time aside” rule. Code review wasn’t a real task that was assigned to someone. Instead, when a pull review was opened, the PM would just ask whoever seemed to have a shorter task queue at the time to review it, whether they were familiar with that module or not, and whether it was a five-line patch or a complete new feature. And, of course, being diligent in your code reviews was a big thing in the annual review, so you kind of had to say something.
First off, that obviously delayed them on the “actual” tasks they’d been assigned (as in the ones on the issue tracker), so they wanted to be done as quickly as possible. More importantly though, “an hour or two” was nowhere near enough to become familiar with the subject matter if you were reviewing a feature (I can’t be too explicit for obvious reasons but the volume of a “feature” was comparable to, say, writing a JSON parser or a minimal DNS server, so self-contained but possibly pretty big).
So most of the time you’d just get a long list of bullshit complaints like “
frameCounterisn’t representative enough because it’s not obvious what kind of frames it counts”. Some people had pet peeves that weren’t in the coding standard but they insisted on (e.g. a singlereturnstatement per function). One person on the team gamed the system in the most insufferable way possible, he just ran the code through a linter and pasted just about every time, slightly reworded so it wouldn’t be obvious. It wasn’t the worst idea, except he ran it with a particular config (one of the first Google results for$linterconfig), so about half of them were either linter noise or just didn’t make any sense in context.Trouble is you literally got so swamped fixing bullshit problems like these that nobody ever got to the real problems. After a few months I found myself literally altering the way I wrote my code depending on who I thought was going to review it just so they’d shut up about whether a loop counter should be called
crtIndexbecauseiis too short oribecausecrtIndexis too long and maybe look at important things I might miss.Worse, because these bullshit review sessions routinely stretched over days and days, people would routinely address these “important” remarks on the run, while already working on something else. On at least three occasions I traced major bugs (as in crashes and/or issues with potential security implications) to typos or off-by-one errors from hastily-written “fixes” that someone had made one the fifth day of pseudo-reviews like these.
To this day I am convinced that that particular codebase would have been better off with no review at all than this charade. I can’t remember a single bug that was caught at review time, but I remember several that were introduced at review time.
I want to add another one:
What I’m saying is, what happened to my Minesweeper quick-marking patch mail, Simon? ;)
I have had a few colleagues over the years that if they didn’t feel qualified to review your patch would just ignore it. Does not matter that they are the only people on the reviewer list. I would have to reach and ask if they were going to get to the review. “Oh, I don’t understand it”.
How it should be handled:
Both of these solutions don’t waste a week of waiting.
I’ve learned to appreciate The Great Silence. Asked in mailing list if a feature was welcome, they said it sounded reasonable, so I wrote the feature, tested it, sent the patches in. One person spent days arguing about the patches with me until it turned out he hadn’t even read my code. Another person called it a schizophrenic feature, and by extension called me a schizophrenic as well for having thought of it.
I’m a regular reviewer of a public open-source project (OCaml), doing reviews as a part-time / free-time activity, and I’m afraid I do most of these things on a regular basis. Oops!
The reason I can think of in most cases is “it turns out this way because I don’t have enough time to spend on the review, so the result is suboptimal but it’s a best-effort”, and sometimes it is “I tried to help by offering some review work for your PR, but in the end I asked more work of the author and the PR is not noticeably closer to being merged”.
More precisely, here is what I can think of as excuses and protections:
There’s a huge difference between reviews in public FOSS projects and reviews in commercial projects. Many of these things are, to some degree, inevitable when the people who send the patches and the people who review them are independent developers working in their spare time, or at best under limited contracts.
But a lot of them are IMHO inexcusable in a commercial setting. Just two examples:
Wherever I’ve seen that happening in a commercial setting, it was a symptom of the management team failing to either understand, or act on their understanding, that the time required to review a pull request is part of a task’s development time. A reviewer who has limited time available for review isn’t some inevitable artefact of the obscure craft of software development, they just have too much work to do for how much time they have.
Many commercial settings have a specific sign-off process for this. If that process requires a sign-off from two reviewers, the author can’t choose the option they prefer :-).
There’s a second important factor at work in FOSS projects. They’re not entirely devoid of “office” politics and the like but most of the time the people working on them enjoy it to some degree. Whoever invested enough time in a project to end up regularly reviewing code is usually recognised by most of the community as being someone you can work with.
This isn’t necessarily the case in a commercial setting. Many of your points are based on you and other reviewers not being jerks. I’m sure you aren’t and that most people who work on OCaml aren’t, either, but working with complete jerks who are also designated reviewers is part and parcel of the corporate life.
The guessing game can be particularly frustrating. The reviewer has a solution in mind and only that solution will unlock the elusive green check. It doesn’t matter if the patch does solve the problem, it doesn’t solve it exactly how the reviewer has in mind and there’s no room for variation. I’ve seen this cause a lot of frustration for the submitter as they slowly make changes, getting closer to what the reviewer is looking for and wondering why the reviewer didn’t just submit the patch themselves.
One of the guidelines I try to follow is not leaving too many comments. If I’m that unsatisfied with the patch, it’s better for both of us to pair on the problem together.
There’s nothing worse than being “done” with a feature and then someone comes with 100s of complains.
Code Reviews should contain only these things:
Non-Blocking nits/ advice. Approve the PR but leave this if there’s just a minor like “yeah it’d be nice if we had some logging here but nbd”. I actually don’t want you to follow up on these in most cases, it’s not important enough but I’m leaving it up to you to decide how to spend your time.
Literal, actual bugs. Actual actual bugs.
Questions about serious, egregiously bad code that I won’t want to be on the hook for. I probably will approve with a “Can you tell me more about this/ I wonder if this could be simpler”.
If you block me on nits I will ignore you, resolve your comment, and wait for someone sensible to approve. You see a 1 line “cleanup”, I see 40 minutes of flaky test failures, CI/CD, context switching, branch switching, and also a 1 line “fix” for something that’s fine as-is.
By far my most common review comment is: “What you are doing here is tricky, and when I come back to the code in N months I will not be able to understand why this is done this way / what the subtlety is. Please write a comment in the source that explains what is going on.”. (Typically this is a matter of transferring explanations that are in the PR description itself into the code.) This not a hard-blocking request, but it is also more serious than the nits you describe, and I expect people to do it.
This is some of the most valuable review feedback. When I’m writing code, I typically have a load of surrounding context in my head. When I read code, I don’t. Someone telling me ‘I think I see how this works, because of this property from over there’ is useful and tells me I missed a very important comment.
I think there are probably some edge cases of “please do this”. Sometimes it’s warranted. I tend to err on not blocking depending on seniority/ experience - if this is a very new person with very little experience I may be less inclined to expect them to have the right context to make those sorts of calls.
I couldn’t disagree more:
Of course, I’m not arguing that the code has to be perfect before it’s merged. As a reviewer, you ought to be considering the long term health and maintainability of the code base, and not just the short term of “does this check the box of this feature?”. I think this is the main difference between a senior developer and a junior, TBH.
That’s a shared responsibility with the author. I’ve advocated for telling the developer these things, like “hey this code is confusing I think it’d benefit from ” or “this could be faster if Y”, but not blocking them on these things because it is a shared responsibility. I’ll only block in egregious cases where I just refuse to be on the hook for maintaining the code because it’s so bad, or literal bugs, etc.
This is a fine ask for, and I would ask for it, but if the code is tested and works and there are comments I’m not blocking on it. Also, if you can’t understand code, maybe you need to learn more idk.
Meh. Unless those conventions actually matter I don’t care and will never block on this, but I’ll bring it up and let them decide. Unless they’re going against conventions that will matter later, like maybe we have some custom Cache construct and it’s important to use it vs some other one.
Doesn’t even make sense. All code is inefficient enough to impact performance. The question is whether the code meets requirements. If it meets the performance requirements the code is valid. If it doesn’t, that’s a bug, and I’ve said to block on bugs already.
TBH I think it’s a very “junior” attitude to think that code is the point. It’s not. The product is the point. Spending a ton of extra time to get the “code” better when you have a working product that meets requirements is just wasting time unless you have an actual product reason to do so (eg: “This will be costly to maintain” is a fine reason). There’s a balance, of course, and as I’ve said before it’s perfectly fine to say “this would be nice” or “I think this could be changed” etc etc etc.
hm, after re-reading your original comment in this light, I think we agree more than I thought. Initially it seemed to me that you have a reckless attitude about accepting anything that passes the tests and aggressively rejecting any improvement suggestions if they sound like too much work to you.
But if you see it as shared responsibility, that makes sense of course. Blocking then sounds more like “I won’t allow this code in at any cost”, essentially vetoing your decision as an author. In our company, we don’t have this kind of blocking / non-blocking distinction really. My team typically resolves all threads in a merge request before merging unless it’s explicitly stated that it’s not important. Not because we want to have the code “perfect” (whatever that means), but because we work on a system which is used in emergency situations, so we also have a very heavy QA pipeline and quite long release cycles. We want to make sure no issues crop up during QA or in prod. So robustness and maintainability is a very strong “product reason” for us, if you will.
I think I may have been overly brisk in my first comment. It’s the shared responsibility bit that matters. The author of a PR is often the one “closest” to their code, and I am closest to perhaps some pre-existing code. That means that I can give context about the code I have the most context on in order to help them understand how their code best fits into it.
But at the end of the day, they are a professional software developer just like I am. I am not interested in telling them how to do their job unless they’re doing something outright wrong. I will give them the context that I have (“this code is unclear to me”, “we usually do this here”, etc) but ultimately they’re an adult and I’m going to let them make the final call on whether they act on that unless they’re doing something that will really fuck things up or just makes zero sense.
If your product requirements are such that you must have very strictly guarded code, then that is perfectly fine. That means that when I see code that is not robust I will have to block that code because it is a “bug” if it does not meet that standard.
Does this function contain a literal actual bug? Assume that, at the specific Git SHA of the PR, there is no obvious situation where a caller of the function passes
42forn.I don’t know. I’d just ask the developer about it.
Reminds me of my experience trying to contribute to LLVM 😂
I’m still hoping to work at a place where I finally get to enjoy the code reviews experience. So far I haven’t. Sometimes it does feel like there’s an element of “power play” in the review process.
Code reviews in private codebases are relatively new. How was the software that still runs the world written before them? They seem (in private codebases) to be the result of organizations no longer trusting their professional employees, testing on end-users, and cargo-culting a process suited to public projects - without evidence of benefit?
hm, I dunno. You could also ask how software got written before without version control, or without automated tests. People got by, but that doesn’t mean that’s the best way to develop software.
At my current company we use code review, and I like it. Not because I don’t trust my colleagues (although to be fair, I sometimes don’t trust myself!), but to increase shared understanding of code and increase its overall quality (yes, in a healthy team, bickering over what name or particular code idiom to use can actually be helpful) and also to prevent issues like inconsistencies, inefficiencies and the occasional actual bug. Often, the conversation on a merge request can also be useful when investigating problems.
At other companies I’ve worked for, code review was often seen as a costly thing to do - why spend the time and energy of two developers on one feature if you can have two developers working on two features? And then people wonder why things are unmaintainable when a developer leaves. Not doing any review (or pair programming, which can fulfill a similar role) also encourages fiefdoms, as each feature will have been built by a single person without much interaction with others.
Whenever anyone asks this kind of question, I am reminded of the STANTEC ZEBRA manual. The ZEBRA had a complex dataflow instruction set. To make it easier to program, it also had a ‘simple code’, which was a subset of common operations. Simple code came with a bunch of limitations, one of which was that programs could not be more than 300 instructions long. This is not a practical problem, the manual assures the reader, because no one could possibly write a correct program that complex.
The world’s moved on since then, but that’s usually the answer: they did it by dealing with far lower levels of complexity. A typical Excel spreadsheet does more complex calculations than anything that ran on the ZEBRA.
Big Design Up-Front has to be the answer for some of that software. Ponderous spec documents, down to naming the functions in a module ahead of time.
I’m not sure about the timeline and whether there’s a period after BDUF but before code review.
Yes, this. I worked as a programmer on payroll software on mainframes in the 1980s. A senior person told me almost exactly what to write AND provided tests. My job was limited to completing the translation of that into COBOL/JCL syntax, compiling, running the tests and double-checking everything.
In case you’ve wondering how the senior person got their work checked: there was one more senior person above them, but they were rarely involved. Mostly if the senior people found anything too difficult then it DIDN’T get done.