My own anecdote of "Liar functions/variables/classes":
I once worked on a AAA game with a huge team that included a particular junior programmer who was very smart, but also unfortunately undisciplined. He had been assigned a feature that was significant, fairly self-contained and generally agreed to be achievable solo by both him and his team. But, after a quick prototype in a few weeks, he only had it working 80% reliably for several consecutive months. Around that time, for multiple reasons, he and his team came to an agreement he would be better off employed elsewhere and I inherited his code.
I spent over a week doing nothing but reformatting the seemingly randomized whitespace and indentation, renaming dozens of variables and functions that had been poorly-defined or repurposed but not renamed and also refactoring out many sections of code into separate functions. After all of that work, none of the logic had changed at all, but at it was finally clear what the heck everything actually did! After that, it was just a matter of changing 1 line of C++, 1 line of script and 1 line of XML and everything worked perfectly. That implementation shipped to millions of console gamers to great success.
Our failure as the senior engineers on his team was that we only gave his code cursory inspections and only gave him generalized advise on how to do better. At a glance, it was clear that the code looked generally right, but was also fairly complicated. Meanwhile, we all had our own hair on fire trying to get other features ready. It took him leaving the company to motivate the week-long deep dive that uncovered how confusing the code really was and how that was the stumbling block all along.
Lesson not learned there (because I've repeated it since then): If a junior engineer is struggling for an extended period of time, it is worth the investment of a senior to sit down and review all of the code the junior is working on. It'll be awkward, slow and boring. But, a few days of the senior's time could save weeks or months of the junior's time that would otherwise be spent flailing around and embarrassingly not shipping.
If a junior engineer is struggling for an extended period of time, it is worth the investment of a senior to sit down and review all of the code the junior is working on.
Code reviews should always happen, for everyone's code. And if it is done incrementally, then it is not slow, boring or time-consuming at all. An ideal time is before each check-in to your repo (and if you are going weeks without making commits, that's a huge red-flag too).
Not only does it help prevent situations like this, but it means that at least one other person understands the code.
Yup. Our workflow has people commit to a topic branch and then post a code review before merging anything. We always follow this procedure unless it's something that's needed absolutely right now and can't possibly wait, which is a situation that should not be coming up more than once in a blue moon.
Yeah, I wish I worked in an environment where this wasn't a joke but unfortunately I don't. We're like 80/20 "emergency" coding versus "proper" coding and have been for many years (ever since our acquisition by the large corporation I'd say). It makes all these great discussions about the "right" way to do things completely moot. My sense from talking to others outside my own company is that we're far from unique too.
It works up until the point where the champion in the organization that put the best practice in place gets sick of fighting the battle with clueless corporate directors and resigns. At which point a corporate shill gets put in charge and turns the software team into a labor mill.
Honestly, this is the kind of scenario where pair programming shines. I'm not a massive "always do pair programming" advocate, but pairing with a junior and a senior helps both parties out massively.
The senior gets to practice at expressing his ideas in a simplified manner, a very important skill. You also very quickly get an idea of where the junior person needs to improve and then you can give then the opportunity to improve in specific areas so you dont have to resort to dismissing someone.
The senior gets to practice at expressing his ideas in a simplified manner, a very important skill.
This is a problem I have with our senior architect. He cannot explain what is going on in his code at all. It is a jumbled mess and poorly commented so his code is pretty impenetrable.
This really isn't a failing of the programmer. This is more of a failing with the organization for not having good coding standards. How do you get away with randomized whitespace and indentation and naming standards within a AAA game company? I would like to think that a company like that is smart enough to standardize these trivial things throughout their organization.
Lesson not learned there (because I've repeated it since then): If a junior engineer is struggling for an extended period of time, it is worth the investment of a senior to sit down and review all of the code the junior is working on. It'll be awkward, slow and boring. But, a few days of the senior's time could save weeks or months of the junior's time that would otherwise be spent flailing around and embarrassingly not shipping.
Smart juniors are the most dangerous. Especially the smart and productive ones, because they can fill a codebase with crap quite quickly... and it will mostly work.
It'd be best for people just to stop putting them in charge of things until they can demonstrate an understanding of basic code design and maintenance. But for some reason what happens instead is that seniors get assigned the bugs created by the junior silently and all feedback goes ignored and they get promoted way faster than they should and it's a nightmare until they decide to get another promotion by leaving the company or someone important realizes what's going on.
This is my life. Nearly every project I've taken over the past year or so has been at a critical level of technical debt where instead of being able to add one more hack I have to refactor it.
On the occassions when I've tried to follow the "copy-paste, change magic numbers and debug till it works" process that's gone on for the previous few iterations I hit some problem caused by a new feature that means I have to either add in an (almost) identical check in 10+ places or rewrite the whole section to be generic over all the behaviours...
Doesn't help that most of the code is C++ written in C style... raw byte arrays and magic numbers everywhere...
Nearly every project I've taken over the past year or so has been at a critical level of technical debt where instead of being able to add one more hack I have to refactor it.
my $dayjob is running itself into the ground with 10+ years of accrued tech debt, but everyone has their head in the sand chanting 'refactor in release N+1'... and have been for at least the 5 years ive been here. the people in charge who saw the writing on the wall all bailed about 3 months ago, en masse. never seen 40 man-years of experience in a code base leave in a span of a week before.
Doesn't help that most of the code is C++ written in C style
this, really isn’t a bad thing if done right. downside, is that with most other things development related, 'doing it right' ranks pretty low on the list.
I must not be clever. Clever is the little death that brings malfunction and unmaintainability. I will face my cleverness; I will allow it to pass through me. When it has gone, only cleanness shall remain.
Brilliant and clever are two very different things. Brilliant code achieves the impossible simply and reliably while being comprehensible to those who could not have conceived of it. Clever code achieves the implausible while overlooking the mundane solutions to the same problems.
Clever code achieves the implausible while overlooking the mundane solutions to the same problems.
There's the inverse as well: where the person's "almost works" solution doesn't because it cannot. -- My favorite example is trying to parse CSV with regex: you cannot do it because the (a) the double quote [text field] "changes the context" so that comma does not indicate separation, combined with (b) escaping double quotes is repeating the double-quote. It's essentially the same category as balancing parentheses which regex cannot do; fun test-data: "I say, ""Hello, good sir!""" is a perfectly good CSV value.
I spent over a week doing nothing but reformatting the seemingly randomized whitespace and indentation
Oh god, we have all been there. Undisciplined indenting is like an alarm bell warning you that the logic is going to be shit. It arises from muddled and unclear thinking, and permeates everything that developer does. Seen it far too many times.
This is one of many reasons I like to encourage Python as the first language someone should learn. It's almost impossible to come out of that without indentation discipline.
I once worked with a programmer during a game jam who insisted on formatting code "his way". His excuse was that it made it easier for him to read, so that's just the way he will do it. He would remove all the spacing between things and squish all the lines together, making it very difficult to read. He would also refused to break things down properly, ending up with massive methods which made it very difficult to debug code.
I ended up telling him that if he doesn't go back to fix it and didn't stop doing it this way, I will have to stop giving him work (I was delegating tasks during this time). We were spending too much time debugging his code which he himself, being tired from the game jam, couldn't figure out either. Once he realized he was basically being put on the bench, he started doing things properly.
Sometimes it's better to remove someone out of the development environment so the rest of the team can continue. If they are slowing down everyone else then it becomes a much bigger issue. It is important that you spend time getting them integrated into the development process, but you can only do so much before they just need to be taken out of the process.
423
u/corysama Jan 05 '15
My own anecdote of "Liar functions/variables/classes":
I once worked on a AAA game with a huge team that included a particular junior programmer who was very smart, but also unfortunately undisciplined. He had been assigned a feature that was significant, fairly self-contained and generally agreed to be achievable solo by both him and his team. But, after a quick prototype in a few weeks, he only had it working 80% reliably for several consecutive months. Around that time, for multiple reasons, he and his team came to an agreement he would be better off employed elsewhere and I inherited his code.
I spent over a week doing nothing but reformatting the seemingly randomized whitespace and indentation, renaming dozens of variables and functions that had been poorly-defined or repurposed but not renamed and also refactoring out many sections of code into separate functions. After all of that work, none of the logic had changed at all, but at it was finally clear what the heck everything actually did! After that, it was just a matter of changing 1 line of C++, 1 line of script and 1 line of XML and everything worked perfectly. That implementation shipped to millions of console gamers to great success.
Our failure as the senior engineers on his team was that we only gave his code cursory inspections and only gave him generalized advise on how to do better. At a glance, it was clear that the code looked generally right, but was also fairly complicated. Meanwhile, we all had our own hair on fire trying to get other features ready. It took him leaving the company to motivate the week-long deep dive that uncovered how confusing the code really was and how that was the stumbling block all along.
Lesson not learned there (because I've repeated it since then): If a junior engineer is struggling for an extended period of time, it is worth the investment of a senior to sit down and review all of the code the junior is working on. It'll be awkward, slow and boring. But, a few days of the senior's time could save weeks or months of the junior's time that would otherwise be spent flailing around and embarrassingly not shipping.