38

My code review nightmare part 2

Team responsible for code 'quality' dictated in their 18+ page coding standard document that all the references in the 'using' block be sorted alphabetically. Easy enough in Visual Studio with the right-click -> 'Remove and Sort Usings', so I thought.

Called into a conference room with other devs and the area manager (because 'Toby' needed an audience) focusing on my lack of code quality and not adhering to the coding standard.

The numerous files in question were unit tests files

using Microsoft.VisualStudio.TestTools.UnitTesting;
using System.Collections.Generic;
using System.Linq;
<the rest of the usings>

T: "As you can see, none of these files' usings are in alphabetical order"
Me: "Um, I think they are. M comes before S"
T: "The standards clearly dictate system level references are to be sorted first."
Mgr: "Yes, why didn't you sort before checking this code in? T couldn't have made the standards any easier to follow. All you had to do is right-click and sort."
Me: "I did. M comes before S."
T: "No You Didn't! That is not a system reference!"
Me: "I disagree. MSTest references are considered a system level reference, but whatever, I'll move that one line if it upsets you that much."
Mgr: "OK smartass, that's enough disrespect. Just follow the fucking standard."
T: "And learn to sort. It's easy. You should have learned that in college"
<Mgr and T have a laugh>
Me: "Are all your unit tests up to standard? I mean, are the usings sorted correctly?"
T:"Um..well..of course they are!"
Me: "Lets take a look."

I had no idea, a sorted usings seems like a detail no one cares about that much and something people do when bored. I navigate to project I knew T was working on and found nearly all the file's usings weren't sorted. I pick on one..

using NUnit;
using Microsoft.Something.Other;
using System;
<the rest of the usings>

Me: "These aren't sorted..."
T: "Uh..um...hey...this file is sorted. N comes before M!"
Me: "Say that again. A little louder please."
Mgr: "NUnit is a system level nuget package. It's fine. We're not wasting time fixing some bug in how Visual Studio sorts"
Me: "Bug? What?..wait...and having me update 10 or so files isn't a waste of time?"
Mgr: "No! Coding standards are never a waste of time! We're done here. This meeting is to review your code and not T's. Fix your bugs and re-submit the code for review..today!"

Comments
  • 6
    Suddenly, some of the stuff I've seen doesn't feel so bad anymore
  • 7
    That's when I would start looking for another job.

    Holy shit, the hypocrisy is unpalatable.
  • 8
    Who cares how Its sorted, as long as Its sorted so related packages are next to each other it shouldn't matter.

    Pointless standard, and retarded people needing validation
  • 4
    Fuck them. Get out of that team
  • 1
    What a dumb assturd
  • 2
    "their 18+ page coding standard document that all the references in the 'using' block be sorted alphabetically"

    What the fuck?

    I get that it's easy enough. But the point is that this is both arbitrary and worthless. You might as well demand all functions be in alphabetical order, or all properties be in alphabetical order (which is almost reasonable!). But using/import statements?

    Find a new job yo.
  • 8
    @user00015 if you have an automated code formatter, it should be easy enough and acts as a brown M&M check. That part I find reasonable enough.

    However, I'm put off by the apparent lack of automation. Code formatting is easy enough to check in CI. Or in a pre-commit hook for that matter. It should never be necessary to bring that up in a code review.

    Of course, then there's their disgusting behavior. Nothing could excuse that.
  • 5
    @PaperTrail

    I really dunno in what SM / Psycho-Terror company you are working... But this is really terrifying.

    Code Styles must be automated.

    There is no if, no but, no except.

    Either automate or let it be.

    If a simple linter cannot understand what the coding guideline does, a human will not be able to do it, too...

    For the behaviour, I as a manager would give HR a hint that someone is ready for either extended training in corporate rules or getting fired.

    There's a distinction between mobbing and doing review ... this is definitely mobbing.

    Which pisses me off.

    Fuck the wankers with an anchor... *Hums Alestorm*
  • 4
    @user00015 > "Find a new job yo."

    Luckily, the administration responsible for the absurdities were fired+quit several years ago.

    Code reviews are now only peered reviewed by people closest to the problem (ex. other members of the same team) and 99% of the time it's only sanity checks. Look for n+1 patterns, hard-coded passwords, make sure errors are logged errors. etc. Nobody cares that there is an extra space between methods (yes, spacing was a *big* issue with the previous admin)

    We use Azure DevOp's code review features, so getting feedback on check-ins nearly eliminates folks looking for attention. That's another TL;DR story.
  • 4
    @iiii > "Get out of that team"

    Luckily, the administration responsible for the code review absurdities were fired+quit several years ago.

    When 'Toby' left, the manager re-assigned me to his position. One of my first (of many) tasks was to delete the code review document from the intranet.
  • 0
    @PaperTrail oh, it's not a present story.
  • 6
    @IntrusionCM > "Either automate or let it be"

    That's where we are at today.

    Nobody says "I tried to understand your code, but you used tabs instead of spaces." anymore.
  • 2
    @iiii > "oh, it's not a present story"

    No, since the exodus of the previous administration, things around here have been really boring.

    Writing code, solving problems, going home..wash-rinse-repeat.
  • 0
    Who the fuck cares about sorted usings?

    Sure, I do that for myself from time to other but i would feel like a total dick if I were to put this in some code quality document or reject PRs because of it 😂
  • 1
    @ReverendLovejoy > "i would feel like a total dick if I were to put this in some code quality document"

    Yea, when I was 'promoted', one of the many improvements was deleting that section.

    I ended up deleting so much of the document, the new department mgr removed it from our document library.

    We currently use SonarQube and it's metrics to improve code quality.
  • 1
    If the standards are this rigorous you should be forced to use some auto format plugin or maybe a commit hook!

    Pretty tiresome to force people to alphabetically sort manually (its good enough for most but if you start having meetings and shit about not doing stuff it needs to be automated as much as possible)
  • 1
    As someone who sorts and groups my import statements according to a dozen different rules like I have OCD, I still think it's ridiculous to make others do that.

    It literally adds nothing to the code. Just makes me feel happier looking at it
Add Comment