The Code Review Pyramid (2022)
morling.dev
morling.dev
To me, code review is not just there to produce better software, but also better developers.
It's a great place to talk about tradeoffs, alternative approaches, reasoning behind changes, etc.
Maybe less useful in huge projects with many contractors you're never going to work with again, but I've seen the mentoring aspect have great benefits in small teams.
I was shocked at my first job to realize the code reviewer didn’t even read my code, merely glanced over and demanded style changes (which were not consistent from review to review in the slightest)
It really felt more like how you hand your advisor your thesis with obvious easy to fix errors so he doesn’t compulsively decide “well something must need improvement” and make you fix something difficult
Teaching people how to design applications is a lot more useful, but code reviews are really not the time and place. For one thing, making someone go back to the beginning when they have a working (if flawed) PR is going to demotivate people and strip away their confidence. It’s also going to really limit your ability to ship quickly. Even if you put those challenges aside, the review experience for GitHub PRs (which I’d guess is how most people are doing reviews) doesn’t allow you to comment on code that wasn’t part of the diff. This makes it really hard to discuss the broader impact of a change i a PR because you can’t add context inline about things someone missed.
After trying and failing to do good mentoring through code reviews I’ve mostly given up. I do use them as a chance to see where people might benefit from some mentoring, but I keep my reviews quick and lightweight. If I don’t see anything wrong with the PR I approve it. Mentoring happens during dedicated time I put on people’s calendars to focus just on mentoring outside of a PR.
I've had quite a few experiences where I've suggested a change in a code review, and found out that the author lacked knowledge in that area. That's then turned into a future topic to focus on in future mentoring sessions.
Agree about design too, I don't think code review is really the place for that, since it's often a bit late by then (plus it's hard to get a sense of the overall intent and architecture from code chagnes alone).
It's a shame it doesn't happen formally more often, but one place I worked in the past always had a separate design review (which needed at least one person to approve) before coding could even start. This was fairly informal, but caught a few major potential design problems early on.
There's an entire industry of body shops that will send you lightly trained 22 year olds, in size.
I've been unfortunate to work with one of those, some years ago. I decided to pretend I enjoyed the style as soon as I realized she suffered from a fragile sense of authority. In hindsight, that doesn't seem to have been a good decision.
It was clear she felt affronted by any deviations from the intended standard. But the feedback mostly comprised extremely vague, subjective, non-actionable ad hoc prescriptions. I carefully read through my reviews and the ones received by my colleagues, but was unable to devise any pattern. Ultimately I decided to simply code as fast as possible so that I could collect the feedback earlier and get rid of the damn thing.
That way I could kind of cope, but after some months I was really burned out and struggling to sustain any interest about my tasks.
My main concern as a reviewer is to make sure future people can understand the code when the inevitable modification comes along.
Things are different if the writer and reviewer are in a mentorship relationship, but even still, if you are only engaging with the code as a mentor when the PR hits, you're messing up the mentorship!
And I personally refuse to work with this style of workflow anymore
Does the code do what it needs? That’s objective and it either does or doesn’t. I can manage that kind of review.
Does the code look pretty enough? This is subjective and guarantees i’ll waste hours of my life every month bc someone else felt anxious/neurotic/masochistic and wanted to dump a chore on me.
No thanks, if you have style guidelines i’ll follow them. I’m not your whipping boy that will ask how high when you say jump.
The fact that something is subjective does not mean that it is useless.
IMHO, writing maintainable code requires a lot of experience, which one cannot expect from all junior developers. Requiring style guidelines for everything is a bit pedantic, and does not even make sense when you stride in to new territories. The latter is increasingly becoming the only part of software development where I would like to go.
If all code and requirements would be so objective and so simple, then I'm afraid you'll soon be replaced by an AI agent :)
‘…readable, maintainable [and tested]’, ie. if someone else needs to modify or remove this code in the future, will there be friction?
The main problem is that those programmers mostly care about superficial syntax and don't actually care about code readability or design. They just go for the lowest hanging fruit and criticise variable naming or whitespace because it is easy; they don't talk about the object graph or the event lifecycle of the program, because that's hard.
A very common thing I see nowadays is a forced adoption of gofmt/black/whatever, hoping it would solve the obsession with the formatting. However, this just locks in a (often substandard) coding style and removes any kind of personality from the code. This is good if you are a manager trying to treat your employees as fungible units of work, but is bad for actually maintaining a codebase. Also, it doesn't stop obsession with style, it just forces a specific style on everyone which most of the time, everyone slightly hates.
This is a really good article which summarises the problem way better than me. https://luminousmen.com/post/my-unpopular-opinion-about-blac...
As with many things it's not actually a technical problem. It's a social problem. And it can work. Where I am right now for example we do have formatting locked in and enforced by linters. And it's not an issue there's no debate about it in regular PRs at all. The code formatter adjusts it and that's that. If you want to change it, create a PR that changes the rules and put a cross section of developers on said PR to discuss it. Happens from time to time but not often and works well.
Or completely disallow tabs, even when it's necessary for accessibility, like with screen readers (not even PEP8 enforces spaces, but black decides to without a configuration option)
Same with whitespace: it refuses you to add more than 1 (or 2, can't remember) empty lines inside a function, even when it would be more readable to space it out.
I heavily disagree with the assumption that consistency is better than readability. I believe that people can be civilised and not change each other's code just for the sake of formatting nitpicks, only when they actually edit the code.
Every project should be able to argue once about style, what is important then just apply that going foward with a configurable style checker.
This is fine. Discussing code style once in a while is not a problem. What's a problem is discussing it at every PR.
Having to endure through asinine style errors that no one in your team chose or even agrees with is worse than spending a couple hours every 6 months discussing a rule or two.
I worked a team where this happened. Black was a godsend
Just absolutely eliminating it as something to worry or argue about is something a lot more languages could benefit from, IMO.
And yes, while other languages have third-party formatting and linting tools that can enforce an agreed-upon standard, in the real world that just never works out as well (once a codebase grows beyond a couple of contributors) as having it be truly standardized in the language itself.
Can you elaborate on this? Once you have a linter/formatter in your CI loop, how does it stop working just because you have more contributors?
My main project at work uses Elixir - which has a built in formatter - and Javascript - which doesn't. We use eslint for Javascript and haven't touched its config in years as devs come and go.
I'm comparing to Rust, and I'm not surprised that there are more different ways to format Rust and not always a way to make everyone happy.
In my experience with people who seemed to confirm this, the opposite turned out to be true. I spent an inordinate amount of time configuring linting and formatting to suit their tastes, to minimize any formatting subjectivity in code review. Their reviews got a lot more substantive and valuable pretty much immediately because their formatting concerns were evidently close enough to solved that they could pay attention to what the code actually did.
I’ve had similar success adopting formatting that no one likes just because it’s opinionated and no one gets to dispute it because they couldn’t if they wanted to without diverting resources to a new tool. Now we’re all unhappy with the formatting and still happy to be reviewing actual substance.
When the bad formatting gets in the way, it’s trivial to ask the question “is this just a whitespace change?” and everyone groans for a second, agrees that it is, and gets on with life.
And when there’s any remaining style considerations (oh you like, or don’t like, reduce?) that’s a much smaller space of style concerns to negotiate and settle.
It’s not that anyone else has felt stifled by this either. Everyone everyone contributing has welcomed the automated formatting dictator. We’ve all felt the benefit of not focusing review on stuff that doesn’t impact users.
If you really want to express yourself in code formatting, go wild on your own time and enjoy the free time you have for it by not putting it in the middle of otherwise meaningful work.
how exactly do you do that "penalising"?
What does this even mean?
Companies don’t (well, shouldn’t) hire developers to just write code. They hire them to solve problems. I’m not going to more easily replaced because my code follows the same styles as others, if I am the one solving the problems they have.
Anyone should be able to follow coding standards, you do not have to be smart for that. You just have to be able to put aside your ego and follow what the team is doing. I am a lot more forgiving to people making mistakes than to people forgetting the coding style.
And anyhow, I enforce the code style by making sure the code simply does not build if you do not follow it.
That's exactly what you want, unless your goal is to have job security rather than building something.
> removes any kind of personality from the code
What kind of personality comes from things like pefering " over '; having tabs over spaces; having n spaces between class methods?
For me, those things are essentially meaningless but nice to have consistency on. Code personality, and a large part of readability comes from things like: - variable / class names - functional vs oop problem solving - type use - custom data structures or not etc
None of those have are locked down or touched by a lint tool like Black. Use it or don't, but you'll have plenty of personality in your code either way.
I don't like that. Code should be a transparent interface, you write something and it becomes part of the program. With code formatting it becomes, you input something into the source code, you transform it with the tool, and then it's done. We sort of lose the brain-hand-program connection.
As an example I randomly found this code snippet of rust imports. To reproduce this by hand would be like writing JSON by hand - I'm referring to the nesting, bracketing and sorting. This is the lack of personality: no longer writing code by hand, but with tools.
use core::fmt;
use std::{
alloc::{GlobalAlloc, System},
cell::UnsafeCell,
cmp,
fmt::Write,
marker::PhantomData,
sync::{
atomic::{AtomicBool, Ordering},
Mutex,
},
time::Duration,
};
use serde::{
de::{self, SeqAccess, Visitor},
Deserialize, Deserializer, Serialize,
};"having n spaces between class methods" is definitely *not* meaningless. It affects code readability and black has very opionated (and often inappropriate) ideas about what counts as "readable" whitespace. When you raise that problem with people, they always say "well too bad, you should write shorter functions instead", which just introduces another layer of opinionatedness.
This is so wrong. There is no reason to not have a standart for formatting code when working in a team.
Of course, that requires a team with emotional maturity, which is I get, not very common in the software industry.
1. Hard disagree on the idea of multiple styles in a single codebase and think being able to identify writers of code by style rather than version control sounds like madness incarnate.
2. Massively want to read a blog post or something from you about your ideas on code style. Sounds like you have some pretty valid and well thought out ideas justying a philosophy of code style I've never heard supported before (I think normally disparate code styles in a code base would get interpreted as a code smell rather than a positive)
Code is not just for style. You get no points for fancy spacing. You get no money for having being a "that guy who likes to format his args weirdly".
Or a lot of changes without reason, making it more time consuming to find the real change in the commit diff log.
I don't mind differences in code style. There is a standart called editorconfig, which IDEs support, which makes code style rules IDE independent.
Actually if you have people with experience and professionals, they don't care about a specific code style rule, they don't care if they switch the code style from project to project, they just want a standart set of rules for the whole team so that there are no issues caused by different formatting styles.
Just stop caring about useless stuff, gee… this is what the code review pyramid about, focus your efforts and care on important stuff, not irrelevant details.
Auto formatted code is the best thing that happened in the last few years. I don’t care that it doesn’t show the « personality of the dev who wrote it », I want consistent code.
> but is bad for actually maintaining a codebase
Absolutely not.
Is it bad, though? Having most of the code in the codebase look and read consistently feels like a good thing, even if that's not the style that you'd personally prefer. Especially if the formatter does its thing whenever you save a file.
Management is right in trying to remove “personality” imo.
I don't really understand your logic - why is management right for removing personality? Of course, from a programmer's perspective, not from the manager's perspective.
From a dev pov, making sure you are easily replaceable means that you could also find a new job easily. So it’s not all bad.
Areas:
- Readability - how understandable the code is
- Maintainability - how the code enables the project to evolve
- Risk - security, regulatory and other vulnerabilities
- Correctness - whether the code does what you intended
- Robustness - how well the code handles unintended circumstances
- Performance - how resource efficient the code is
## Readability
Code is readable if it meets your expectations and surprises you only when there's something you don't yet know about the technology. We expect code to look like the code around it. To follow most, if not all of the technologies idioms. To use clear and informative names. To explain why deviance exists. To be as abstract as the concepts being abstracted are crystallised.
## Maintainability
Maintainability is the code's relationship with the wider team and time. You should be able to learn all you need to know to change, test and deploy code. You should be confident that any changes you make will not cause unintended changes elsewhere in the code. To make a change, you should have to touch as little code as the concepts being changed are crystallised. You should be able to debug an issue as easily as, the system the issue is in, is old. You should be able to track down and alter the changes that introduce a bug quickly. You should be able to learn why a change exists.
## Risk
Code is low risk if it accounts for, and where possible, mitigates the various risks to a project.
## Correctness
Correct code does as you expect and surprises you only when there something about the system you didn't yet understand. Validating correct code is as automated as the validations are frequent. Correct code is only as complicated as the concepts it models. Correct code is only as abstract as the concepts it models are crystallised.
## Robustness
Robust code handles unexpected circumstances safely, quickly and predictably. Validating robust code is as automated as the validations are frequent and as comprehensive as the failures are risky.
## Performance
Performant code is as resource efficient as the resources are expensive, but also as efficient as the development costs are cheap.
but, for example,
Writing good tests massively contributes to good implementation details and API semantics; and test code is also code that needs to be reviewed under more-or-less the same criteria as the rest.
Also - documentation (or, legibility, if you're onboard with self-documenting code) can be more important than either implementation details, and even API semantics, as it can define whether the entire work is useable or maintainable.
I might say instead:
- What will it be like reviewing this code? (style, test coverage, etc - do I have to worry about spotting typos, and other stupid stuff?)
- What will be like debugging this code? (patterns, logging, etc - which might handled by a framework)
- What will it be like altering this code? (documentation/legibility, implementation details, etc - when the business needs change or grow)
- What will it be like using this code? (API semantics, API docs - when I go to build something on top of this)
And then yeah; the top should be entirely automated, and you should (generally) spend most of your time on the bottom.
When a feature is implemented I preferably want the entire thing before me when reviewing. Otherwise I find it hard to keep track of everything that has happened.
Not to mention that the small focused changes might be reviewed by different people leaving only the implementer in full knowledge of everything that was done.
Peer/Assembly programming help but if you do peer/assembly programming reviews are mostly a waste of time (especially for assembly programming).
Although, the style guide should just be followed and fixed before you get to code review phase. That’s just a matter of professionalism
We had to adjust our linter settings here and there - but it was still super efficient for everyone's time compared to what we had before...
I can't recommend this more.
The Code Review Pyramid - https://news.ycombinator.com/item?id=30757206 - March 2022 (110 comments)
The Code Review Pyramid - https://news.ycombinator.com/item?id=30674159 - March 2022 (4 comments)
I think this pyramid is accurate but better framed as a "review pyramid". Catch semantic issues in an earlier phase (informal discussion or rfc-style proposal document).
Sure, but the price is worth it. If we have a design session before you start coding, it definitely will make the feature go “late” by an amount of time proportional to the design session… which is less time than the required to fix the design after the PR is open.
“Fast moving engineers”: I can only imagine managers advocating for that. As an engineer, I can only but express disdain for such a mentality.
Code style should be automated same as running tests when someone creates PR.
As, IMHO it depends (as so often). Code review is commonly the place that puts the current style under test (hence automated otherwise you don't have the results during review) and under review.
E.g. Code review finds out if a code-style is missing, incomplete or even outright wrong.
If that approach is taken, passing tests and code-style must be in there, otherwise it is not useful to argue about violations. From my understanding I would therefore see those results be available during review.
It may also be that there is code-style for automated test-code, and that code part of the review. Without taken this into context of the code-review, I miss a bit the boundary of your comment.
Could you elaborate a bit where you draw the line and why? E.g. I can imagine there is a benefit to keep a distinct context for code-reviews so that they are still practically feasible and those parts that need further adoption are put into a different phase, like steps before (preparation of the current increment) or after (preparation of next increments) the code-review itself.
Discussing or fixing code style is huge waste of time and styling can be automated. Yes it will be broken in some edge cases but for me that is acceptable because my goal is to deliver the feature and not to make "perfectly aligned code". Aligning code by hand is waste of time, thinking about aligning code is waste of time. Formatting should be in configuration and you format whole file at once and never do stuff by hand.
Passing tests is also easy to automate when creating PR, CI should run tests and tell that to person creating PR - hey tests failed, fix it and then after they pass you can open PR, not a fellow developer because that is waste of time for everyone.
I did not write "tests should not be there" only "are all tests passing?" is not part of code review for me, it is something that is there before code review even begins.
So checking if there are proper tests and if tests make sense is part of code review.
Nevertheless, things can be different, e.g. if CI passes, it merges already, no need to have a request for merge, which are often a blocker to keep a steady development flow. Naturally, this benefits from tests that are already run during development, if not driving the development.
And I didn't read you're against testing nor aligning white space and comments at all, more the localization. Actually the points you raise are looking important to me, because if alignment comes in late, this can cause a lot of erosion, which can hinder any review if not even provoke merge conflicts which are stopping the process quite early and require re-iteration.
Your approach should also work towards non-release blocking code reviews, a property I personally like, as I've seen teams struggle with code reviews, becoming more and more of a burden, even after practicing it has found its way. But that is only a subjective comment of mine, every project is different, which makes it an interesting topic for me.
What's surprising to me in the article is the pyramid style. In all business school and psychology texts, the top of the pyramid is the most important one. Here it's the least important.
Same with joining new team, one might have preferences but you leave them at the door and format code with whatever already is there.
There is tooling where you can also have formatting configuration per repo so if you use IDE it would take that config and format it for you in "project style".
The benefit of automated linting is that the codebase becomes more coherent and easier to read.
The down side is that you can’t add your personal flare to your contributions… but in my opinion that’s not too important
And again, being against personal flair only makes sense if you are a manager trying to treat programmers interchangeably, but as a programmer, your interests are increasing personal flavour in code so you are less replaceable.
It took me 6 months to fully decode all his personal mnemonics, abbreviations, style decisions and bizarre architecture plans.
There were several times during that time when I considered trying to find him to tell him what I thought about him to his face.
You are going to be replaced at some point, unless you think this company is going to fold before you retire.
If he actually documented his abbreviations, mnemonics, style and architecture, you would have most likely appreciated it instead of cursing it, I think.
So replaceable in fact that the company wouldn't have had to hire me at great expense and could have got a normal developer in to take over and continue.
Maybe I'm just extremely jaded since my job is fixing messes left by people with more style than sense but I see striving for uniqueness to be a flaw in an engineer.
> If they have dress code or code style it is for them to set it up.
I also chose this company because they don't. A dress code, what power-hungry lunacy is that?
Anything that makes it harder to determine correctness is a strike against the code. This can include anything from high level organization, to comments, to the naming of variables, to the use of whitespace. For example, with respect to variable names, if you're implementing something that has to conform to the DICOM standard, then you should consider following their nomenclature, even if it may not be what you'd normally use in other contexts.
Aside from correctness issues, I'll make anything from "strong recommendations" to "mild suggestions". (In theory, any of these could cause me not to approve a change, but the closer we get to "mild suggestion" the less likely that is.) Examples might include:
- You're trying to do something with X. I'm a domain expert in X and while your code is technically correct it's not idiomatic. I suggest doing it this other way instead.
- You added a public function to a library, but it will fail if the String argument isn't a valid Photometric Interpretation. Yes, I see that you only call it with valid values, but since it's public anybody can now call it, and they may not be so diligent. How about making it an Enum?
- You have a loop that, superficially, looks like it's calculating Bar, but upon closer examination it's (correctly) calculating Bar'. How about adding a comment stating this, so that no one is tempted to (incorrectly) "fix" it?
- You've implemented a function that does Y. All our code links with the Foo library, which happens to have a function that does Y. How about using that instead?
- Your log message makes sense if you're reading the surrounding code, but someone from support seeing it in the log file won't know what to make of it. How about including this contextual information in it?
- In the loop that you added, your indentation is inconsistent with the rest of the function. How about making it the same?
- Why do you have four blanks lines in the middle of your function?
In my own code I'm pretty anal about formatting, comments, log messages, use of whitespace, etc. I'm definitely less harsh when reviewing other people's code, but I have to admit that when I see sloppy formatting, grammatical errors or gratuitous use of whitespace, I'm on the alert for sloppiness in other areas, such as design and implementation.
Sure, unit tests require much more maintenance than higher level tests, but they give you confidence that the smallest parts of the codebase work as intended. They're the ones that test all sorts of failure scenarios and edge cases, which is typically not the purpose of integration tests. They also should be inexpensive to run, simple to write, and require minimal setup.
Unit tests should also give you immediate feedback that something went wrong, by pinpointing the exact component that failed. In contrast, integration tests might happily pass, as they're not granular enough to cover all code paths. Integration tests focus on, well, that the integration between components is working as expected.
So I still insist on having mostly unit tests, many integration tests, and some E2E tests. Doing it otherwise because it saves you maintenance efforts will haunt you in the long run.
Most of us are writing glue code that isn't as clear what the api should be. Unit tests just get in the way of refactoring when requirements change.
But if you delay adding unit tests until after the interfaces are more or less stable, it gives you higher confidence to refactor the implementation, which is invaluable.
On the other hand, the benefit of TDD is that it helps with the actual API design. You immediately experience the API as a user, and this process drives the implementation, and ensures you end up with a friendly and testable API. If you add tests after the design, then you might have a more difficult time refactoring the implementation to make it testable, and the DX could suffer.
So I see both sides of the argument, but in either case, unit tests are very important, and I wouldn't avoid them just because they're a chore to maintain.
You can so TDD using only integration tests. This means tests are not added after the design. It only means that your individual functions don't have tests. I do this allowed the time, it makes refactoring easier.
The most common objection: how do you know what failed? Turns out to be a non issues, since I make my tests fast I can run them often and thus the failure has an obvious cause: the last thing I changed.
Some projects have almost zero code like this.
In all other situations they suck. When they fail it doesnt necessarily mean anything is wrong and vice versa. It just mirrors a portion of the code. It's a lot of expensive maintenance with no payoff.
In contrast integration tests are usually much more meaningful - they will typically test a real system or user interaction.
Integration tests are important for features, unit tests stay important for code blocks. My realization was that a ton of what we wrote aren't features, but blocks.
For instance, let's imagine an API that build an invoice. Where do you put the VAT calculation test ? Do you jam your 100+ VAT test cases in the integrations tests, or do you unit test the VAT calculation in isolation and only cover the main cases in the integration ones ?
Same for name display, invoice number generation, Invoice items fetching etc. If you want a decent coverage and also document/properly test the edge cases, unit test will easily be half or more of your whole test volume. And of course you'll want to reuse there blocks, so make sure they're rock solid, so pay even more attention to tests.
If you test unit interfaces and not implementation details then that problem goes away.
Unit tests just help to make sure that the units you have right now are doing their jobs. Obviously if you change what units you have or how responsibilities are allocated among them then you will need corresponding changes in the unit tests.
Other people disagree: <https://blog.thecodewhisperer.com/series#beware-the-integrat...>
In short (IIUC), integration tests do not give you any feedback on when your design needs to change, while unit tests do.
Don't be fooled by your peers that look for superficial things to comment on. There are many people with the same amount of knowledge as you, but who are less self-aware and are adding noise the system, because they are scared of being perceived as incompetent.
Find someone who has given you meaningful feedback during code/design review, and see what kind of comments they leave on other people's work. If you don't understand one of their suggestions ask them about it.
Good engineers figure out the important stuff before anyone writes a line of code. The APIs, the architecture, high-level component names. It's all been talked over. When a good engineer reviews another good engineer's pull request, they're just checking that what is delivered is an implementation of what is expected.
Code review is a road block for people who don't do the above. It causes them to bump into someone who knows what's up.
Everything else that gets argued about: local variable names, formatting, equivalent ways to express the same thing. It's not important. I don't know if it's actually confusion about what matters, or if it's a desire to make people jump through hoops, or maybe boredom?
It's also worth not spending too much time on creating the perfect design upfront, since it might change during development. The whole waterfall vs. agile situation. An initial proposal document helps with avoiding lengthy discussions and friction during code reviews, but it also takes a lot of effort to produce, and even then, reviews can be difficult for many reasons.
> I don't know if it's actually confusion about what matters, or if it's a desire to make people jump through hoops, or maybe boredom?
My theory is that sometimes it's a need to say _something_ to prove that you've read the code. Also, egos are often involved, and some reviewers have the need to demonstrate their superior intellect/knowledge/whatever by insisting on minutia. Software engineers are often proud and take code reviews as a chance to flaunt their intellect, while at the same time being defensive of their opinions, and argumentative to the point of arrogance. Few senior engineers are humble, and consider code reviews as a collaborative effort to deliver the best possible solution.
> Few senior engineers are humble, and consider code reviews as a collaborative effort to deliver the best possible solution.
The difference between these two only that in first case author didn't agree with reviewer and agreed in the second.
The really worst type of review that approve without looking or IDC.
sometimes as people who care about our craft, we need to step back and keep the big picture in mind. the goal is to make a functioning website, or video game, or text editor. the user couldn't care less about your code formatting, composition vs inheritance decisions, or commit messages. these are useful tools to the extent that they make it easier for yourself or others in the future to provide more value to the user.
at my job, people (mostly external contractors) might make pull requests with formatting annoyances, pointless null checks, getters and setters that have no reason to exist, useless comments, "== true", things of that nature. I'd ask these to be fixed and sometimes they would be, but other times someone else would just come in and approve the PR themselves. I realized one day that not a single one of those ugly PR's that made it through have ever come back to bite us in any way. everyone's time was better spent continuing to work on providing value to the user instead.
I hold myself to a higher standard, but when reading others' code, I've found it really means nothing to me where they put the brackets or what naming convention they use, or even if they change these up randomly. I care that it does the thing, has no glaring red flags, looks appropriately performant for the problem at hand, and won't be a maintenance nightmare (will spending 30 minutes changing something right now save at least 1 hour dealing with it later? will those code even be around by the time we get to that point, or replaced by something else? in many cases probably not, of course consider how foundational or isolated the code is.) in all my experience, any bikeshedding over aesthetics has proven to be a total waste of time.
if we're making a thing that lets the people designing a website make a thing for the visitors, I care that the experience is good for the designer and the visitor, it does what it should, etc. and it's reasonably maintainable, but I'm not going to drag someone into the bikeshed and beat them with the tire pump of subjective aesthetics until they rename their variables to my whimsies.
If the reviewer wants me to add an unnecessary `try... catch`, I simply do that. No point arguing with someone's ego, which takes way more time. If the reviewer is asking me to code for the eventuality that Google will suddenly change their the JSON model in their API response? You got it bud. You want to preserve some convention without explaining how it applies here? I will squish my code to it, keeping it correct.
I know it makes my code less readable, more complex. But I care about delivering the value as early as possible. Also the code is not my baby, it's the company's baby. I try to fight comlpexity in the design phase, while also ensuring the ego folks get to have their optics.
Aside from useless comments, if someone opens a PR with any of those things, I can only assume it’s because they don’t know why they aren’t necessary. As their colleague, I feel compelled to tell them, because personally I cringe at the thought of doing things that aren’t needed and nobody having the courtesy to tell me otherwise.
A “deep review” involves completely understanding the code, verifying that it works, refactoring sections or even completely rewriting the original. This type of review can easily take 30 - 70% of the original effort.
A quick spot check type review involves rapidly scanning the code for code standards or anything egregiously incorrect.
There doesn’t seem to be much of a sweet spot as either the cost is too high or the value is low.
I think most reviews are closer to “spot check” type territory. Maybe this can just be done by AI and deep reviews can be done on an as needed basis.
Perhaps we can eschew the code review and simply expect that developers do high quality work. When this doesn’t happen, processes could be in place for needed fixes post commit - obviously needed anyway since most reviews just scratch the surface.
This is a bit of a strawman, but I expect most have felt that in-depth reviews require too much time while quick spot check type reviews add little value and interrupt everyone’s flow considerably.
it makes me wonder if the product/engineering split has grown wider than it should. we really should be pretending we're product managers more.
Tests very much in particular deserve more weight. Being DRY is nowhere on the same level as bike shedding code style.
Or even just immediately. I often find minor issues just by looking at it in a different context (the merge request vs my editor).
Does this apply to prototypes too?
If you are a team of two, and once of them is on holiday, do you wait until they come back?
If you think you’re smart enough to submit code without bugs and without peer review, that’s fine. Take into account that your peers may not think you are that smart if you do that. Software engineering is mostly about dealing with people (well, almost everything in this world is about that).
Obviously if there’s no one to review your code, you may as well go ahead and push it without approval (unless your company has policies against that).
mandatory code review is a symptom of an adversarial environment involving bad faith actors.
there is no quick fix for organizational disfunction, and in that environment mandatory code review will have a negligible effect.
and is required (afaik) for PCI compliance