2. They could but they usually don't. It's fine to sit down and make sure they understand some esoteric structure within a system I owj
I'm not going to sit down with someone else to make sure they use for each loops unless it's a very new junior (i wouldn't block that in most code reviews anyway unless the code was legitimately painful to read, but every company is different).
3. IME there may be multiple reviewers but one primary reviewer. Half the time the non-primary ones either focus on "is this code going to break your work flow?" or are simply there for bookkeeping and isn't the approval you're looking for (i.e. A lead on a review that already talked about the plans but doesn't have the knowledge to review that specific module). Anymore than 1 primary reviewer probably needed a smaller PR if possible.
Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me. Here’s the thing: there’s no such thing as “standard” software architecture (until someone writes a new kind of software architecture called “standard”).
I’ve seen and used everything from MVC to DDD to TDD to MVVC to whatever React is to reactive to event-oriented to actors to whatever you can imagine. There simply isn’t a “standard” way to do anything in this industry. If you hired me, you hired me for these different perspectives on problem solving, and that goes for all of us.
A code review simply isn’t the place to enforce your unique perspectives on someone else.
Sometimes, there is simply a better way to do things. These might even be new ideas in the code base. If it looks like it is, hopefully the author will bring it up with the team beforehand. If the reviewer changes their mind in the code review… well, that’s not the authors problem. The reviewer should rewrite it and the author review it.
Actually, that’s exactly what I suggest if someone ever tries that with me (usually when I’m the new guy). Please submit your own PR showing me how it’s done. I’ll move on to another ticket.
Usually, about half way through their implementation, they’ll see why I did it the way I did it and approve my PR. It only takes one time before they start asking “why” instead of assuming they know everything; every line of code exists for a reason, after all.
In one case, one specific dude (bless his heart) kept blocking PRs after we discussed everything before hand. I had to get HR involved, and other engineers. Dude just didn’t like being wrong and would throw tantrums when he was. He eventually got fired after people realized they could do what I was doing to stop the nits he said was a blocker. Nobody cared if a variable should be renamed from “SameFactory” to “EqualFactory”.
Ah, but code quality! It’s so subjective. High code quality, to me, is easy to maintain code. Is the intention clear, do the comments reflect reality, is it easy to read, but more importantly, easy to change? Will changing a line in the module break 15 other sibling modules? God I hope not. Dependencies should be obvious. And, no, I’m not writing an interface if there is exactly one implementation. That’s ridiculous.
For some people, they want layers. More layers of abstraction than a layered cake on a wedding day. To them, that’s good code quality! Some people think high code quality is beautiful code. The kind you frame and put over your fire place to admire with a glass of wine.
There’s no objectively “high quality” code in existence.
Yeah, gotta love it when someone uses terms like "standard" and "quality" but definitions of those are written nowhere. Quality is almost never objective either; some cultures like "overripe" bananas and some consider the best bananas to have no spots, and those picking bananas need a different basis for what makes a good quality banana to the buyer. Unless programmers diligently document their standards (which they rarely do adequately), they shouldn't be surprised if the other programmers is dumbfounded by being told their code isn't up to standard.
I think there is a such thing as “bad code” but not low quality code. Bad code can be detected by automated tooling and be improved through simple refactoring.
I'd say it this way: bad code doesn't robustly handle all the use cases, mis-interprets inputs sometimes, or is a mess to read and debug.
Yes, it matters what the code looks like. Knew a guy, named everything in his code a letter of the alphabet. a,b,c and when he got to z started za, zb etc.
That was 'bad code'. Or, it was 'Low Quality Code'. Nobody wants junk like that. Even if it passes tests, is robust etc.
I can think of a few tools that do that to your PHP or JS code, on purpose. They even cost quite a bit of money (obfuscation). Sounds like you might have gotten a good deal there ... /s
>Anyone who has ever come along in a code review and told me my code “isn’t up to standard” will inevitably start a very long game of attrition with me.
standard is relative, and while I'm talking generically as someone on the internet with no context of your experience, a code reviewer shouldn't. Someone literally saying "the code is not up to standards" needs to clarify.
For my generic take, take "standard" as "whatever that team has laid out in its code architecture". Sometimes there may be times to question the standards if it compromises other important factors, but personally I don't think the code review is the time to fight over whitespace and bracket placement (those should be solved via a linter anyway). Make the correction and submit, those later discussions can happen offline.
>A code review simply isn’t the place to enforce your unique perspectives on someone else.
Likewise, the goal is to align everybody, not to argue over semantics or enforce your own POV. That's why code style sheets are a thing; it's a mediator that can serve as its own battlground should team/company style need to be changed. Again, every place is different, but
1. style should be automated as much as possible. If there's some stupid whitespace rule, it should be a one button click on my IDE from some provided linter to resolve (even if I will proceed to re-lint it to my style afterwards on my local machine).
2. style sheets can have suggestions as well as laws. Be reasonable on what is what
>For some people, they want layers.
sure, lawful evils exist, both maliciously and inadvertently. Sounds like you ran into both kinds. Office politics are inescapable even at the best companies.
That sounds like another discussion to adjust to style sheet, not throw it out upright. If you see a bunch of examples of a law being broken, adjust it to a suggestion unless is a strong argument made otherwise. (most) companies aren't a congress where amending such things takes months of proposal, and we can very much adjust the document to the people if there's no resistance.
Now, do you actually care enough to spearhead that change? I imagine many don't, and there in lies the problem. That's why I'm not on management track; I don't have the care nor attention to worry about documenting such things unless someone throws it at me. I'll leave that to those who do care.
To be clear, I'm not talking about code style (easily objectively quantifiable) or bad code (also easily objectively quantifiable), but architecture/design. This can be things like which folder an interface belongs in, where to put a method in a parent/child relationship that touches both, when to use value objects vs. scalars, when to break functionality into another service/library, which methods are allowed to call which methods, which classes must be injected vs. instantiated, etc.
>The reviewer could have sat down with the person before a single line of code was written.
This is a bigger waste of time. If every change requires asking someone in person, that's an infinite source of distraction and kills productivity for that person.
MOST code reviews don't end up in comments asking for big changes, and when they do the one-time cost on the author is much better to pay than the constant tax on your subject matter experts of what you're proposing.
How is spending 5 minutes x TEAM_SIZE to go over what you're going to do more time than 1-2 days solving a problem, then another hour or two for the reviewer to write out why it's wrong, then another day or two rewriting everything from scratch?
* Reasonably-sized CLs take 5-10 minutes to review - if I have things to say. Much less if the code looks good and I don't have any requests.
* I cannot think of any CL in the last 5 years where I made a comment that caused someone to "rewrite everything from scratch". Most feedback can be done in a few clicks in an IDE which supports refactor or a few minutes of copy/paste.
Generally, it goes something like this imaginary Slack convo:
Me: I think we are getting events from SQS out of order and that’s why we are seeing some weird synchronization issues. What do you think about sending events to the regular queue and a FIFO queue at the same time and comparing them?
Team: How would that work?
Me: Since we are using a single threaded consumer here, I think we can simply override the transport class and instead of pulling from one queue per transport instance, we set up a feature flag allowing us to pull from two, compare the events and if they are different, alerting us in the logs.
Team: Sounds good!
You don’t need a full design spec, just agreement from the team on the approach. There won’t be any surprises in the review.
There can always be surprises. Sometimes when you see something implemented you can get a « click » as to why another solution would be better. It happens to me regularly both on the receiving and giving end of this and it’s rarely a big deal. Most of the time it happened to me, I could reuse the functional tests I had written and parts of the code anyway.
Reworking on something so that it’s better is never a waste of time, and it’s always better to do it before it reaches Prod.
Making a suggestion on how to do something better is quite a bit different than saying “this isn’t up to standard. Do not pass go. Do not collect $200”
We all see these suggestions. That’s ok, and wanted. But to say “ah, no I think it should be the other way.” [reject]
For starters, it results in a waste of time for literally everyone involved:
1. The person writing has to rewrite it (probably).
2. The reviewer could have sat down with the person before a single line of code was written.
3. Anyone else reviewing just wasted their time because it will be rewritten.
If you see code badly designed, walk over to, or call the developer and say "hey, can we discuss the design of the code."
The saying "measure twice, cut once" doesn't just apply to wood. Design before you ever write any code.