Coding is like handwriting.

Code review is about having a common understanding of the big picture and ensure be features follow the general architecture and process flow.

Code review is not about nitpicking on FUCKING TRAILING WHITEFUCKINGSPACES , lower case vs upper case SQL statements, extra empty lines AND EVERY FUCKING MINOR DETAIL you can imagine

But it is...

Consistent coding style makes the code easier to manage as the longer you work with it the more you see it subconciously. Two blocks of text separated by 1 newline are methods. 2 newlines -- inner classes. Big letters hardcoded as a string - sql queries. Small ones -- prolly some logging statements.

I mean every char becomes some kind of an indicator over time. Not to mention consistent and clean code looks more professional. And we want to be professionals, don't we?

I'm not talking about irregular casing or two and three spaces between characters.

Coding is 90% reading existing code and 10% writing. If you know that ten other poor fuckers will read your code and 9 other blocks before they find what they're looking for, and you can choose to write it so it looks like the 9 other blocks, why wouldn't you?

Pretty and consistant styling and patterns are incredibly important. Arguably more than function because someone else will then be able to untangle your mess, figure out what it's supposed to be doing, and improve it.

If everyone wrote functional-but-hideous messes, the codebase would turn into an unreadable pile of trash. You might as well read and extend decompiled or minified code.

I worked with a bunch of intern-level game devs who did this crap. Every var was a single letter, started with conv_ because they saw it somewhere, or their code confusingly used a particular global everywhere because they likewise saw someone else using it and thought it was a magic keyword. Some others outright refused to use ANY whitespace, others had {} in random places, etc. There was absolutely no consistantly styling or form, and basically none had any idea what a pattern was. Some people even used different versions of the language (that had radically different syntax) because they were more used to it, thought learning the newer one was too much effort, and "v1 does everything just fine so why do you even care?" 😡

Back-talking interns* aside, trying to add functionality was a large undertaking, let alone trying to fix their bugs without rewriting bloody everything.

* I was the lead dev and/or dev manager. I happily fired most of them.

@ReverendLovejoy
> I'm not talking about irregular casing or two and three spaces between characters

What are you talking about then? When I read "like handwriting" I think of "everyone has their individual style, and some could be mistaken for chicken scratches".

@VaderNT @ReverendLovejoy

Unless you are working on your personal project, there MUST BE NO SUCH THING as _individual style_.

There is an agreed project code style and no other styles must be seen there.

Consistency. CONSISTENCY. C O N S I S T E N C Y !

@netikras I know. But apparently that's missing the actual point @ReverendLovejoy tried to make?

I'm not talking about individual styles, nor naming variables ctr_a, ctr_b or adding random braces anywhere.

I'm taking about nitpicking. Like

if(isTrue == true)

VS
if(isTrue == true)

Anyone noticed the trailing whitespace there ?

Or

const string someString = ".."

VS

var someString = "..."

in a simple fucking string passed once to a method and never to be used again

Or

public void Foo()
{
//Foo
//Bar

return fuck;
}

Oh, no, why did someone use two linefeeds there? Now my world will collapse

@ReverendLovejoy you are right. All of that shouldn't be part of a code review. It should be part of the Linter/auto formatter you use in your project which automatically fixes all of that or forces you to fix it before you can submit a merge request.

@ReverendLovejoy I fucking hate empty lines inside functions. If it's supposed to separate logical blocks then those should be in separate functions.

Those are the “brown M&M’s”.
https://insider.com/van-halen-brown...

It’s the same idea, if you aren’t careful with things that don’t matter (like white space) then I can’t trust you to write things that do matter.

I fucking hate mixed whitespace and trailing whitespace. The first breaks indentation for everyone but you, and the latter is just stupid.

You literally can't make a case for having trailing whitespace. It has absolutely no point, and only serves to a) add mess, and b) increase file size.

I would likewise reject a PR with either of these. It's slop, and shows that you simply don't care -- or missed it when committing. And before you bitch, it takes literal seconds to fix.

Also, a static string reqlly should be a const, not a var. Not rejectable, but I would absolutely point it out (and then fix it myself), and would start rejecting your PRs if it continued. Blank lines within functions isn't rejectable, either, unless it's really ugly like having a blank every other freaking line. Some of the devs I work with do this, and it's fucking awful to read. Code should be readable and understandable at a glance. Anything that makes that task harder should be fixed or stripped out.

@bkwilliams That is absolutely brilliant

@ReverendLovejoy and now I open your file, instinctively hit ctrl+alt+l and.. Oh shit.. I wasn't going to change this file, but autoformatter made it to comply to our coding style and made whitespace changes. Now I will have to commit and push your file only because of whitespaces :/ even though it has nothing to do with the bug I am fixing atm...

And my ide is now highlighting that missing const part of yours, because it does not comply with our coding style. Should I leave this warning just sitting there? Is this file of yours such an exceptional case so everyone in the team would make an exception for it? Or do I once again walk after you and make the code comply to our standards, because you couldn't and made all our ides have a warning? Yep.. I will do that side commit for your code. Instead of working on my task I'm stashing and wasting my time doing things right, because you couldn't. Or weren't willing to.. Now additional 10 minutes wasted begs a question - wtf is wrong with that team member of ours who cannot use an autoformatter/stylecheck/linter? Is it worth having him among us if he cannot comply to our requirements? Is he worth us wasting our time fixing his code because he could not comply to our coding style standard??

FYI we have whitespace markers enabled in our ides, so yeah, an ide will show that extra whitespace. What are you coding in, notepad?

Srsly man. Project code style is very important. You must comply to it at all times. You want to be thinking about code flow instead of "why the f is that whitespace there??" or "wait, it is not const.. Where is he updating this var?? I can't find it! "

This did not go as well for you as you might have thought, did it?

And yes I agree with everyone here, fix your fucking code. No white-space! And use your empty lines properly! And const static vars only!

Its not that hard to comply to lint and the coding standards of your team, if your code is the only code that gets reviewed like this, you are the problem, not the code reviewer.

@Drmzindec you're absolutely right.

And no... my code isn't the only code reviewed like this.

Code review should also be about codestyle.

@Root I use that analogy for new devs. But I’m going to have to find a newer band as some haven’t heard of Van Halen.