395
cdrice
3y

This code review gave me eye cancer.

So, first of all, let me apologize to anyone impacted by eye cancer, if that really is a thing... because that sounds absolutely horrible. But, believe me, this code was absolutely horrible, too.

I was asked to code review another team's script. I don't like reviewing code from other teams, as I'm pretty "intense" and a nit-picker -- my own team knows and expects this, but I tend to really piss off other people who don't expect my level of input on "what I really think" about their code...

So, I get this script to review. It's over 200 lines of bash (so right away, it's fair game for a boilerplate "this should be re-written in python" or similar reply)... but I dive in to see what they sent.

My eyes.

My eyes.

MY EYES.

So, I certainly cannot violate IP rules and post any of the actual code here (be thankful - be very thankful), but let me just say, I think it may be the worst code I've ever seen. And I've been coding and code-reviewing for upwards of 30 years now. And I've seen a LOT of bad code...

I imagine the author of this script was a rebellious teenager who found the google shell scripting style guide and screamed "YOU'RE NOT MY REAL DAD!" at it and then set out to flagrantly violate every single rule and suggestion in the most dramatic ways possible.

Then they found every other style guide they could, and violated all THOSE rules, too. Just because they were there.

Within the same script... within the SAME CODE BLOCK... 2-space indentation... 4-space indentation... 8-space indentation... TAB indentation... and (just to be complete) NO indentation (entire blocks of code within another function of conditional block, all left-justified, no indentation at all).

lowercase variable/function names, UPPERCASE names, underscore_separated_names, CamelCase names, and every permutation of those as well.

Comments? Not a single one to be found, aside from a 4-line stanza at the top, containing a brief description of that the script did and (to their shame), the name of the author. There were, however, ENTIRE BLOCKS of code commented out.

[ In the examples below, I've replaced indentation spacing with '-', as I couldn't get devrant to format the indentation in a way to suitably share my pain otherwise... ]

Within just a few lines of one another, functions defined as...

function somefunction {
----stuff
}

Another_Function() {
------------stuff
}

There were conditionals blocks in various forms, indentation be damned...

if [ ... ]; then
--stuff
fi

if [ ... ]
--then
----some_stuff
fi

if [ ... ]
then
----something
something_else
--another_thing
fi

And brilliantly un-reachable code blocks, like:

if [ -z "$SOME_VAR" ]; then
--SOME_VAR="blah"
fi

if [ -z "$SOME_VAR" ]
----then
----SOME_VAR="foo"
fi

if [ -z "$SOME_VAR" ]
--then
--echo "SOME_VAR must be set"
fi

Do you remember the classic "demo" programs people used to distribute (like back in the 90s) -- where the program had no real purpose other than to demonstrate various graphics, just for the sake of demonstrating graphics techniques? Or some of those really bad photo slideshows, were the person making the slideshow used EVERY transition possible (slide, wipe, cross-fade, shapes, spins, on and on)? All just for the sake of "showing off" what they could do with the software? I honestly felt like I was looking at some kind of perverse shell-script demo, where the author was trying to use every possible style or obscure syntax possible, just to do it.

But this was PRODUCTION CODE.

There was absolutely no consistency, even within 1-2 adjacent lines. There is no way to maintain this. It's nearly impossible even understand what it's trying to do. It was just pure insanity. Lines and lines of insanity.

I picture the author of this code as some sort of hybrid hipster-artist-goth-mental-patient, chain-smoking clove cigarettes in their office, flinging their own poo at their monitor, frothing at the mouth and screaming "I CODE MY TRUTH! THIS CODE IS MY ART! IT WILL NOT CONFORM TO YOUR WORLDLY STANDARDS!"

I gave up after the first 100 lines.

Gave up.

I washed my eyes out with bleach.

Then I contacted my HR hotline to see if our medical insurance covers eye cancer.

Comments
  • 82
    Wow, that's a hell of a rant. I hope you get the treatment you need
  • 29
    afaik this_style_of_writing is called snake case :)
  • 9
    @SHA-256 True 'dat. I waffled on "snake_case_names" but I over-thought that might lose some readers and distract from the rant-flow. In retrospect, I probably should have left it as-is... err, as-was? ;)
  • 23
    Sounds like he didn't write any of it. Just cobbled together other folks' code.
    Would explain the naming and indentation madness
  • 7
    @mhudson that was what I was going to say. Seems obvious.
  • 11
    Well ranted. I am interested to know what happens next. Does he defends the code against review does he runs away in shame or does he not learn anything from this review?
  • 2
    Sounds like most of the issues were syntax. When the script is concatenated and served - does it work?
  • 6
    @yendenikhil The group he's in claims to have "extensively reviewed" it and wanted our opinion. I call BS. I'm not putting my name on it, nope nope.
  • 12
    @sheriffderek Not just syntax; silly logic errors as well (the unreachable code above for a trivial example).

    I'm not sure you necessarily meant it that way, but "Does it work" is a very low bar to set for production code. This rant is actually related to another one of my previous rants about some code which destroyed an entire cluster of servers. If the intent was to completely destroy servers, sure, it "worked" great.

    The over-arching point of my rant (as well as my refusal to put my name on the code review) is that production code needs to be held to a much higher standard than "Did it work in the test environment".
  • 2
    @cdrice agreed - but just curious
  • 3
    Nice rant, and btw I always thought in camel case you weren't supposed to capitalize the first letter.
  • 5
    @calmyourtities you aren't. Capitalized first letter is TitleCase or PascalCase iirc
  • 2
    @iam13islucky oh, yay! Terminology isn't really my thing, although I do like to correct people on hacker and cracker 😂
  • 2
    @iam13islucky oh yeah and the double comment thing happened to me too. I guess it could be the app, I kept thinking it was a problem with my screen.
  • 4
    @calmyourtities @iam13islucky
    I wondered if anyone would call me on that -- good for you, you are my kinda people. ;) The whole camel case classification is a little debatable, and a lot of people (myself included) use camel case to refer to the whole suite of "sub-cases"... just to be lazy, really. You'd think I'd use PascalCase / camelCase more formally, given a lot of my early programming was actually _in_ Pascal, hahaha, but maybe I'm subconsciously just trying to forget those days...
  • 7
    copy-pasta from a lot of places, and not using a normal editor to format.
    this happens a lot when an IDE dependant coder (coders?) tries to write a bash script.
    make them rewrite the whole thing in python, and that will take care if the indent problem. 200 lines of bash are 20-40 lines of python...
  • 2
    Not really related but whenever I read rants like this, I think: well, at least you are working with Unix. I'm forced to work with Windows, and write my shell scripts in Cygwin. Hell, people here don't even know what shell scripts are.

    Also, sounds like some people could really benefit from using shellcheck 👍
  • 2
    @Neftas One of my team-mates was teaching himself PowerShell and having quite a hard time.

    Me: Why don't you just use bash in cygwin?

    He: What's cyg... cyg-what?

    Me: [ deep sigh ]
  • 2
    It sounds as a bad case of unattended copy-paste.
  • 4
    Eye cancer is a real thing.

    I recently heard of 2 interesting cases, a boy (blind) who would click his tongue for echo location.
    And a guy (blind) who plays street fighter.

    The boy died, the cancer came back.
    But the street fighter thing was last week.
    He even won some rounds, imagine getting beaten by a blind person in street fighter. I guess sight actually blinds is and blindness sometimes let us see
  • 3
    Well, imo you should let him know his code was very, and i mean VERY pleasant to read and review. Tell him he is unique and got a bright future ahead of him... After all this, write

    class Sarcasm():
    print("This is sarcasm!")
    Sarcasm()

    Here i hope you learned two things now
  • 4
    @Strazil

    def sarcasm(sarcastic_message):
    message = '%s - Yeah, what they said!\n\n:P' % sarcastic_message
    return message
    print(sarcasm("This is Sarcasm!"))

    > This is Sarcasm! - Yeah, what they said!

    :P
    >
  • 2
    Signed up to ++! Angry and verbose is my favorite :)
  • 1
    I just got the chills & I'm still at indentation.
  • 1
    ~$ if [ -z "$aeo" ]; then aeo=a; fi
    ~$ echo $aeo
    a

    -----

    Seems like the examples of unreachable code are actually reachable?
  • 1
    @bitshift The _first_ stanza is reached, but the subsequent tests (repeatedly testing if the string is empty) are not, since the first stanza assures the variable is set to a value (thus it will _not_ be empty for subsequent tests).
  • 1
    @cdrice I see :) Thought it was different examples.
  • 3
    This sounds like madness, who doesn't write comments in a long piece of code? What if it doesn't work out, and you have to back and fix it? And, bash, of all things. Bash is good, but messy. I can't imagine what you had to go through.
  • 1
    Eye cancer does exist. And my Gramps lost an eye from it. Don't worry though, mate, it's all good!
  • 1
    @cdrice I almost don’t dare to ask, but would you consider reviewing some/one of my bash scripts? The combination of brutal honesty and 30 years of experience that you have would be a huge learning opportunity for me! 🙊
  • 1
    @bashlord Sure, I'm always down for looking at code. If you're serious about making/keeping it clean, first make sure it passes lint tools like shellcheck. (Linting is arguably more popular for other languages, and seems to be grossly overlooked for shell scripts).
  • 1
    @cdrice Great! Never heard of shellcheck before, nice tool. I ran it over my scripts and removed a couple of quoting errors that I made. It would be great if you could
    take a look at the following scripts. I understand that looling at all of them is a lot to ask, know that I won't blame you if you only look at one of them. After all, your poor eyes need their rest after what you saw :). The scripts are part of a tiny web framework that I am writing for bash (I know, I know 🙃 ).

    the scripts:
    BashtronListener.bash:
    https://github.com/redrock9/...
    ServerGenerator.bash:
    https://github.com/redrock9/...
    Template.bash:
    https://github.com/redrock9/...

    If you are curious about their usage you take a peek in App.bash, which is an entry point for a proof of concept app that I made:
    https://github.com/redrock9/...
Add Comment