> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools;

I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect a lot of people, don't have a lot of time, not my department, etc.)

One of the things I hate is having a different comment for every single place where I misplaced a star or ampersand. My natural tendency is to do it one way, but our style guidelines prefer it the other way, so it's something I miss frequently. Having 30 comments in a review that are all "move the star to the other side" over and over again just makes me hate the reviewer. (Particularly if they don't provide any useful feedback.) A single comment on the first one that says something like, "This doesn't match the style guidelines, and I see several others that don't match. Please fix them all," would be highly preferable. And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!

Everyone is talking about how to get the style guide integrated into your CI. But it sounds to me like your concern is that other reviewers frequently point out your style guide violations.

You could set up some local lint rules just for yourself. You'd need to set it up to only run against code that you changed. And if you don't have an automated style guide you'd have to come up with your own. But it seems like once you get over the hump you could just slowly add rules as you get tired of forgetting about them/people pointing them out to you.

> And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!

This is a concerning attitude. If the team has agreed on a set of style guides to follow then you should follow them, argue to change them, or move on.

If you don't have time to adhere to the style guide now, why do you think you will have more time to go back and fix it later?

Their concern isn't that other reviewers point it out. It's that they think they shouldn't have to follow the guidelines:

> My natural tendency is to do it one way, but our style guidelines prefer it the other way

Which is fine if it's a reason it's an occasionally slip up. But apparently, in spite of knowing this tendency, they frequently submit code to review where they've messed up to such a degree that reading every instance is considered annoying. Think of the poor reviewer who had to put all the comments in!

Well that's a very uncharitable way to read what I wrote.

I previously worked on code bases where it had to be one way. It became pretty engrained in me. Now it needs to be the other way. I try my best to use the "new" style now, but because I'm human and busy, I sometimes forget. I spend most of my time focusing on the problem I'm working on and not how I'm placing characters on the line.

I'm not the only one, so I don't think I'm being a particularly bad programmer. We have one reviewer who's really concerned about star placement. It's sometimes the only thing he points out, and he points it out on a lot of different people's work, so I'm certainly not worse than anyone else on the team. As I say, I do try, but sometimes I forget.

The other thing is sometimes I didn't write the code in question. I hit the issue where I'm refactoring and I move a chunk of previously-written code that didn't follow the guidelines, but I didn't modify the code I moved, so now it looks like I "forgot" but in reality, I just moved some code around. How much time do we want to spend reformatting code instead of working on the actual problem we're trying to fix or implement? I would love if we could automate this, but see my other comment about the issues with that.

I think these are all understandable things that busy people have to deal with. It's not that I don't think I shouldn't have to follow the rules. It's that some rules are much more important than others, and differences in code that have the same readability but don't in any way change the functionality seem less important to me than things that can actually cause bugs.

In addendum, if a couple of style guidelines don't apply to a couple of lines of code for a couple of people, would it have any meaning at all?

Sorry, but formatting is on you. If you don't like the comments, don't commit inconsistent code. Set up a local auto-formatter with the right rules. If enough of the existing code you're working on doesn't conform, set the formatter to only run manually and get in the habit of running it before committing.

Yeah, I've been looking for something to do that, but haven't found something that fits into my workflow yet.

Tools are easy. You add it to the CI/CD pipeline, it fails if it doesn’t meet the guideline, people have to run the tool to get the merge in. On first run it’s painful, so if you’re not starting a project, use an autoformatter so it’s not all manual work. If you really want, you can even make your CI pipeline push commits with autofixes

Of course if you don’t have a CI pipeline that’s easy to integrate steps like this, you’ve got major issues to deal with elsewhere…

I don't have the permissions to add it to the CI/CD pipeline. I've filed bugs on our CI/CD to add it, but it's apparently not a high priority (you know, until someone sits down to review code, then suddenly it's the most important thing in the world).

If I were you, I’d be looking to move on from your company then. Developers should own the testing part of the pipeline at the very least.

Set up the linters to only lint code that was changed. This way legacy code doesn't need to be fixed all at once, but will slowly get fixed over time

Yes, please. It's awful when a one-line PR changes the whole file to fix formatting

Something as simple as a pre-commit hook that checks files that were changed in your commit and rejects the commit if the rules aren't followed is a very cheap and effective way to enforce a coding style.

For a python project for example, you could set pre-commit to check formatting, linting and typing, which means that when your code reaches the remote repository, before any CI is even started or any PR is openned (before you even push really), you know your code follows the rules that have been set for the project. Thus everyone knows you don't need to spend time in the code review looking for these mistakes and instead focus on the actual features.

pre-commit does not even need to be pushed to remote to work, so if your company has no plan to enforce it but it still helps you, you can still use it, unlike a CI.

As for this: > And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!

It's true that one mistake is easy to fix later, but it's just as easy to fix now. If you postpone it once you will postpone it again when it's not highlighted in a PR diff, it's just a recipe for never fixing it. Every language has automatic linters/formaters that check (and even fix) your code according to rules you define, turning the work of finding these mistakes into a simple `lint ./src/` command. It's takes a small amount of time to setup but saves you a lot of time down the line.

I love the idea of a pre-commit hook. I did actually implement this for a past project, and the rest of the team hated it. Whenever they thought it was being "unfair" to them, I'd get email about it because I wrote it.

The problem with my current project is that it has millions of lines of code and that code all has different styles. So even if you only check the lines that actually changed in a pre-commit hook, you can end up with different styles in the same file, which can be as bad as using the wrong style. At least if it's consistently wrong, working within a single file is doable.

> It's true that one mistake is easy to fix later, but it's just as easy to fix now.

Eh... maybe. I see your point, but I guess what I'm saying is "choose your battles." Every minor thing one points out like this takes time away from me fixing bigger issues, so be judicious in how much of a stickler you want to be for the rules. Yep, I can fix it now pretty easily, and then I won't be working on the next bug until it's fixed. And the way we have GitHub set up, if I make any change, I have to get at least 1-3 reviewers to re-review the code and re-approve it. That's the real time waster because everyone else if fixing the nit-picky things for their latest pull request. If they take too long, then I have merge conflicts and I have to address those before I can commit, and, oops! that causes the review clock to start over. We get to do it all again! Talk about Kafka-esque.

If we were starting a new project, I would probably demand that we start with some sort of pre-commit formatter that forces the appropriate style onto our code, but we're unlikely to be doing that for the foreseeable future, unfortunately.

>(Particularly if they don't provide any useful feedback.)

If my eyes stumble on formatting violations every other line, you're probably also not going to get good feedback from me.

Code formatting isn't very important, yet it is.

I've been on a project where we dynamically generated tons of different Cypher queries. To keep those queries readable, consistent formatting was very important, and I had to keep bringing that up in code review.

I hated it. It absolutely should be automated, but I could not find any tools for formatting Cypher query strings inside Javascript code. I'd probably have to write it myself.

I haven't yet taken this concept to completion, but when I had a similar issue years ago I hacked out a cheesy program to filter out static analysis messages on lines not modified by the diff:

https://github.com/NateEag/diff-check

Maybe someone's built the complete version out there somewhere?

Are you sure it's not in your ability? Whenever I introduced something like that people were in general indifferent-to-happy, as long as you did the majority of the job and they could somewhat easily run those tools (e.g. anything written in Java is more or less a lost cause because people cling to their IDEs)

> * Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposing the reasoning behind the demand;

I think this depends a lot. I do "readability" reviews at my company to make sure people apply good coding standards and adhere to the coding guidelines that we have. A lot of these are very "mechanical" in nature and when I am reviewing changes that have obvious coding standard mistakes I will write comments like a "linter" would. One of the most common examples:

> Use descriptive ("Uses") rather than imperative ("Use") style for function docstrings. See: <url-explaining-docstring-guidelines>

I think this type of "giving orders" in reviews is fine as long as it's clear this is an obvious misuse/misunderstanding/miss of the readability guidelines. For any other kind of comment I'll phrase it differently ("Is there a reason why you did X? I think doing Y might be more appropriate because <reasons>, what do you think?", etc) but for stuff that linter should've caught I don't think it's necessary to be verbose and "sweeten" the comments too much.

> I do "readability" reviews at my company to make sure > people apply good coding standards and adhere to the > coding guidelines that we have.

Consider avoiding the entire formatting arguments as part of the code review process. There are excellent linters for every language that can run as part of the check-in process. GitHub + Actions can automate every aspect of this using all turkey stuff.

Automating where possible avoids all the arguments, actually enforces standards, and saves time for everyone. Using "Standard" linting rules (aka: The defaults) also help promote good practices for everyone.

> Consider avoiding the entire formatting arguments as part of the code review process.

That is not going to help me if people send code to review that has formatting problems. We catch a lot of stuff using automated tools, but there's some stuff that you cannot catch. Some people declare every variable as global rather than private, and then I have to follow up with "This variable/constant should be private, any reason you marked it as external?" etc.

You can't just automate everything, and we also have our own specific code formatting guidelines that a lot of tools don't automatically detect by default. I have raised some of these points to our toolchain and engineer productivity teams but it's not my job to dig deep into our tooling and automation so until there's better support in our internal tools (we don't use github + actions) to detect that, I'll keep pointing out any formatting mishap that gets through our linters.

Where I work there is a readability review like mason55's (probably the same company). Most of the comments I see are not about formatting. They're more like "Take parameters by std::string_view instead of const std::string&" or "Use a reference member instead of a pointer member if it'll never be null."

Applying a linter to a giant company-wide monorepo can be difficult because of decades-old code that predates certain style rules, or some teams that might have created their own rules. Even if the linter only applies to diffs, a small change to a line can trigger a lint warning on that line, and having a ton of those can be annoying.

I agree in general, though I find the second on clinging to code difficult when people counter with timelines. It can be difficult to express to juniors how to reason about taking a bit more time to do it "right".

Even when you express it clearly, many juniors won't understand and will feel frustrated. Some things can't be taught and must be experienced. Every junior developer must work on a crufty legacy codebase to develop understanding and habits for writing maintainable code.

Large companies pick experienced internal transfers for shiny new projects and hire externally for unglamorous legacy systems. Since all new grads are external hires, they end up starting out on legacy systems.

I recommend that junior devs start out at Amazon. They will learn a lot of important things in a year or two: the pain of technical debt, the value of automated tests, the pain of production issues when you're on call, that working overtime is not rewarded and does not solve job insecurity, and that switching companies is a great way to get more experience and earn more money.

That's good advice!