Everything a computer can do trivially should not be done by humans. There are plenty of tools available to validate code to all the standards you can dream up. And most languages allow you to even automatically fix style errors or even enforce it for compilation (thank you Go).
So instead of codifying it in a style guide document, codify it in config files for linting and autoformat tools instead. The defaults of those tools are often fine for what you need and exceptions can as easily be documented in them as well.
Also one example on how to codify almost any style is using something like Sonarqube SSLR (https://docs.sonarqube.org/display/DEV/SSLR) where the code is converted into an abstract representation where comprehensible rule can be written for (instead of for example a bunch of regexes). But you really have to be deep into wanting to enforce every aspect of a style to dive into this one.
Agreed, to a point. In many contexts, it is important for humans to do things themselves occasionally in order to learn how, even if a computer is generally used to do those things far more quickly.
Do you think there is any value in writing the code with the correct style before it is fixed in the compiler?
Like is it easier to help out and spot a problem quicker, if everyone is thinking in the exact same patterns when they write code?
An effective team is comprised of people with a variety of backgrounds and approaches.
I don't have to worry about bikeshedding/yak shaving anymore and now can focus on more important problems.
if a = foo()
if (a = foo())
Even in reviews where the reviewee had some amazing code that was kind of groundbreaking in one area, still have to find something to gripe about because I have 20 years on this kid. This seems to be quite common in science/engineering, not just in code reviews.
I wish I could find that company where nobody gives a shit about their place in the hierarchy and everyone isn't constantly trying to gun for other people's jobs to make their linkedin profiles look more impressive. It's been a looooong time now, still searching.
The problem we do have though is huge queues for qualified reviewers and getting randomly allocated a reviewer somewhere on the other side of the planet so that even if they have minor comments (e.g. trivial typos in comments) you're looking at a 48 hour turnaround to get your code checked in sometimes.
I guess for the age/hierarchy thing, it could be possible to make the reviews double-blind via the tooling? I.e. reviewers dont know who they are reviewing, and the reviewee doesn't know who is reviewing them. Obviously once the code is committed then everything would become clear and transparent. Might help in larger orgs, although I dont doubt that it would be fairly easy to make an educated guess who is writing the code a lot of the time, and you'd be able to dig into git/whatever to see the details, but just hiding it in the review tool might go a long way to prevent unconscious biases. If someone has a real axe to grind is deliberately going out of their way to find out who wrote the code during a double-blind review so they can make special comments then you probably have a bigger problem.
I honestly don't know which failure mode is more common. I do know that I had the latter problem in spades when I was earlier in my career and that I'm really paranoid now about being that senior person on a power trip.
But if everyone is trying really hard to both act in good faith themselves and assume good faith in their coworkers, careful reviews are the best way I'm aware of for getting great code into the tree.
I wish I had someone with decades more experience than me taking the time to find areas where my code could improve. Regardless of their motivation, this sounds like an amazing resource that you're lucky to have.
Let's say you send code out for review that is just about perfect how it is. The reviewer is faced with a choice. If they don't suggest any changes, they may feel it gives the impression that you are just good a coder as they are, that their input isn't needed, or that they didn't bother to read it and just rubber stamped it. All of these challenge their place in the organization as a technical thought leader and influencer.
Some people have the maturity and awareness to just say, "Great job. I see no reason to change any of this. Approved." Some people don't, so they will dig for something negative to say. Even if it is just "these variable names are excessively long, and it's distracting to read" or "you don't have enough parameters to warrant the use of the Builder pattern here, so take it out".
One of the questions I try to ask myself with any comment I write as a reviewer: "Would I accept this comment?".
This attitude often completely breaks the effectiveness of the whole process, making it a deal breaker for quick iterations and agile projects.
> I wish I could find that company where nobody gives a shit about their place in the hierarchy and everyone isn't constantly trying to gun for other people's jobs to make their linkedin profiles look more impressive.
This does not resonate at all.
They definitely exist (I work at such a place) so I wish you the best of luck in your search!
- bugs found in code reviews MUST be fixed
- coding style violations MUST be fixed (ideally all of these are caught by lint but not all are)
- everything else is treated as optional
Some code review tools even provide the ability to specify required vs. optional feedback. We now use github reviews which doesn't have this feature so we usually just add OPTIONAL to any review comment that isn't in the MUST fix category.
Occasionally a reviewer and reviewee will get into discussion/disagreement over the approach a particular code change. In those cases if it isn't clearly a bug or a style violation we err on the side of the reviewee.
It's easy to forget that we're still learning how to write "good" code, and that many of our best practices continue to evolve, occasionally into VERY different things. A Code Review has to be able to identify poor practices as distinct from UNUSUAL practices, with that latter evaluated in terms of cost-benefits and not forgotten even when the costs are too high in the current project - that may be a practice to try out where there's less impact to being inconsistent. A Code Review has to balance the subjective opinions of multiple devs to try and achieve some consensus that is treated as objective. None of this is easy nor truly figured out, and not being a jerk is just one of multiple steps to accomplish.
I think that there's a lot of value in having somebody who has a picture in their head of the entirety of a system who can spot impedance mismatches across projects and spot germinating code smells before they cause major problems.
This is your subjective interpretation of the facts. Learn how they complain (in the end, they only can complain about technical facts: learn them, and be correct next time), and you will not receive complaints anymore.
I'm guessing you're new to code reviews right?
More than anything else, though, I'm curious where teams like this still go to buy those amazing Apple Cinema displays?!