Join devRant
Do all the things like
++ or -- rants, post your own rants, comment on others' rants and build your customized dev avatar
Sign Up
Pipeless API
From the creators of devRant, Pipeless lets you power real-time personalized recommendations and activity feeds using a simple API
Learn More
Search - "brief review"
-
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.32 -
My first job: The Mystery of The Powered-Down Server
I paid my way through college by working every-other-semester in the Cooperative-Education Program my school provided. My first job was with a small company (now defunct) which made some of the very first optical-storage robotic storage systems. I honestly forgot what I was "officially" hired for at first, but I quickly moved up into the kernel device-driver team and was quite happy there.
It was primarily a Solaris shop, with a smattering of IBM AIX RS/6000. It was one of these ill-fated RS/6000 machines which (by no fault of its own) plays a major role in this story.
One day, I came to work to find my team-leader in quite a tizzy -- cursing and ranting about our VAR selling us bad equipment; about how IBM just doesn't make good hardware like they did in the good old days; about how back when _he_ was in charge of buying equipment this wouldn't happen, and on and on and on.
Our primary AIX dev server was powered off when he arrived. He booted it up, checked logs and was running self-diagnostics, but absolutely nothing so far indicated why the machine had shut down. We blew a couple of hours trying to figure out what happened, to no avail. Eventually, with other deadlines looming, we just chalked it up be something we'll look into more later.
Several days went by, with the usual day-to-day comings and goings; no surprises.
Then, next week, it happened again.
My team-leader was LIVID. The same server was hard-down again when he came in; no explanation. He opened a ticket with IBM and put in a call to our VAR rep, demanding answers -- how could they sell us bad equipment -- why isn't there any indication of what's failing -- someone must come out here and fix this NOW, and on and on and on.
(As a quick aside, in case it's not clearly coming through between-the-lines, our team leader was always a little bit "over to top" for me. He was the kind of person who "got things done," and as long as you stayed on his good side, you could just watch the fireworks most days - but it became pretty exhausting sometimes).
Back our story -
An IBM CE comes out and does a full on-site hardware diagnostic -- tears the whole server down, runs through everything one part a time. Absolutely. Nothing. Wrong.
I recall, at some point of all this, making the comment "It's almost like someone just pulls the plug on it -- like the power just, poof, goes away."
My team-leader demands the CE replace the power supply, even though it appeared to be operating normally. He does, at our cost, of course.
Another weeks goes by and all is forgotten in the swamp of work we have to do.
Until one day, the next week... Yes, you guessed it... It happens again. The server is down. Heads are exploding (will at least one head we all know by now). With all the screaming going on, the entire office staff should have comped some Advil.
My team-leader demands the facilities team do a full diagnostic on the UPS system and assure we aren't getting drop-outs on the power system. They do the diagnostic. They also review the logs for the power/load distribution to the entire lab and office spaces. Nothing is amiss.
This would also be a good time draw the picture of where this server is -- this particular server is not in the actual server room, it's out in the office area. That's on purpose, since it is connected to a demo robotics cabinet we use for testing and POC work. And customer demos. This will date me, but these were the days when robotic storage was new and VERY exciting to watch...
So, this is basically a couple of big boxes out on the office floor, with power cables running into a special power-drop near the middle of the room. That information might seem superfluous now, but will come into play shortly in our story.
So, we still have no answer to what's causing the server problems, but we all have work to do, so we keep plugging away, hoping for the best.
The team leader is insisting the VAR swap in a new server.
One night, we (the device-driver team) are working late, burning the midnight oil, right there in the office, and we bear witness to something I will never forget.
The cleaning staff came in.
Anxious for a brief distraction from our marathon of debugging, we stopped to watch them set up and start cleaning the office for a bit.
Then, friends, I Am Not Making This Up(tm)... I watched one of the cleaning staff walk right over to that beautiful RS/6000 dev server, dwarfed in shadow beside that huge robotic disc enclosure... and yank the server power cable right out of the dedicated power drop. And plug in their vacuum cleaner. And vacuum the floor.
We each looked at one-another, slowly, in bewilderment... and then went home, after a brief discussion on the way out the door.
You see, our team-leader wasn't with us that night; so before we left, we all agreed to come in late the next day. Very late indeed.9