DevDisasters

Pink Slip for the Developer Who Slipped In

If you think Bert is all talk when it comes to his decade of C# experience, you're wrong. He delivers…15 times.

• By Mark Bowytz
• 04/02/2015

Brian wasn't sure how it happened or what had changed.

When he was hired on five years ago, the process ahead of his being included into the fold of Initech's (not the company's real name) developers was a bit of a pain, though not an uncommon one. After all, you have to be rigorous in your search for the very best.

You see, before being offered a shot at an on-site interview, Brian first had to prove his mettle by completing an online exam. Then he had to fight his way through a gauntlet of phone screenings with several development leads. Only then was he invited in for a full-day interview process with everybody that he had spoken to and an HR drone to make sure he was mentally and morally fit for the position.

Even after he was hired on, Brian had to earn his "Initech .NET Certification" before being allowed to deploy code into production, and prove that, even as an experienced new hire, he could successfully and correctly work within the corporate source control and release environment.

But somehow, the hiring process was relaxed for Bert.

A self-proclaimed 10-year C# expert, Bert's resume had managed to magically impress Brian's team lead and lead architect, allowing him to be fast-tracked through the interview process. Some say that he must have employed some sort of mind trick by talking non-stop about various patterns and the like, but the consensus was clear -- everybody who interviewed him came back feeling that he had the "it" factor they were looking for. Bert was on-boarded in record time and thrown right into the mix, assigned to projects even before being "certified."

But how good was Bert, really?

15 Times Over
A year into the "Bert Era," Brian finally had a chance to experience Bert's code first-hand after it came time to add a new version of some third-party application to the company's flagship app's supported list. This was functionality added by Bert around a few months before.

Brian did not get very far when the WTFs started racing through his head like a Class 5 hurricane.

Turns out, even with 10 years of C#, Bert didn't seem too fond of arrays:

public const string SUPPORTED_VERSIONS = "7.8|7.9|7.10|7.11|7.12|7.13|7.14|8.0";
public const string SUPPORTED_RELEASES = 
  "(R2009a)|(R2009b)|(R2010a)|(R2010b)|(R2011a)|(R2011b)|(R2012a)|(R2012b)";
private const int MAX_VERSIONS = 15;

So, not only was Bert using a pipe "|" as a string separator, but also, it would appear that the application would only support 15 versions. Ever.

Now comes the horror where, around 50 lines later, Bert decided that arrays might actually be a good thing:

private string [] m_ReleaseArray;
private string [] m_VersionArray;
public VersionconstraintsSettings()
{
  id = "Supportedconstraints";
  m_VersionArray = SUPPORTED_Supported_VERSIONS.Split('|');
  m_ReleaseArray = SUPPORTED_Supported_RELEASES.Split('|');
  if (m_VersionArray.Length != m_ReleaseArray.Length)
  {
    System.Diagnostics.Debug.Fail(
      "Please make sure m_VersionArray and m_ReleaseArray match (size and content)");
  }
  var toolboxarray = GENERAL_TOOLBOXES.Split('|');
  m_SupportedtoolboxCombinations = new SupportedToolboxCombinations(m_VersionArray);
  m_SupportedtoolboxCombinations.AddNewToolBoxes(new List<string>(toolboxarray));
  m_SupportedtoolboxCombinations.Apply();
}

"OK, so, we have an array…could've done that in the first place, but whatever," Brian remarked out loud, "Now we would like to enable/disable certain versions, of course, so let's create a variable for that, and all other 14 possible supported versions":

private UndoRedo<bool> m_EnabledZero = new UndoRedo<bool>(false);
private UndoRedo<bool> m_EnabledOne = new UndoRedo<bool>(false);
private UndoRedo<bool> m_EnabledTwo = new UndoRedo<bool>(false);
...
private UndoRedo<bool> m_EnabledFourteen = new UndoRedo<bool>(false);

"And surely we would need an accessor for every value!":

public bool EnabledZero
{
  get
  {
    return m_EnabledZero.Value;
  }
  set
  {
    if (m_EnabledZero.Value != value)
    {
      object oldValue = m_EnabledZero.Value;
      m_EnabledZero.Value = value;
      if (!value)
      {
        m_SupportedtoolboxCombinations.UnCheckSupportedVersion(0);
      }
      PropertyHasChanged("EnabledZero", oldValue);
    }
  }
}

"Ugh ... repeated 14 more times? And if I want to see if a specific version is enabled ... are you kidding me!?":

public string VersionEnabledZero
{
  get
  {
    return GetEnabledVersion(0);
  }
  set
  {
    // Not needed, just for data binding
  }
}
public string VersionEnabledOne
{
  get
  {
    return GetEnabledVersion(1);
  }
  set
  {
    // Not needed, just for data binding
  }
}

... and so on through to VersionEnabledFourteen.

Brian groaned, "Or, wait, I know -- I guess ignore common sense all together and do something like this...":

private bool GetVersionEnabled(int index)
{
  switch (index)
  {
    case 0:
      return EnabledZero;
    case 1:
      return EnabledOne;
    case 2:
      return EnabledTwo;
    ...
    case 14: 
      return EnabledFourteen;
        ...

...and again. Brian started wondering if the number 15 actually held some kind of cosmic significance for Bert:

private void ResetVersionEnabled(int index, bool val)
{
  switch (index)
  {
    case 0:
      m_EnabledZero.Value = val;
      return;
    case 1:
      m_EnabledOne.Value = val;
      return;
    case 2:
      m_EnabledTwo.Value = val;
      return;
    ...
    case 14:
      m_EnabledFourteen.Value = val;
      return;
    ...

And then there's the mother-of-all-case switches to see if all versions are supported:

public bool AllVersionsHaveBeenEnabled
{
  get
  {
    bool res = (PresentedVersionZero == "") || EnabledZero;
    res = res && ((PresentedVersionOne == "") || EnabledOne);
    res = res && ((PresentedVersionTwo == "") || EnabledTwo);
    etc...
  set
  {
    if (m_Lock_AllVersionsHaveBeenEnabled)
    {
      return;
    }
    if (PresentedVersionZero != "")
    {
      EnabledZero = value;
    }
    if (PresentedVersionOne != "")
    {
      EnabledOne = value;
    }
    if (PresentedVersionTwo != "")
    {
      EnabledTwo = value;
    }
    etc...
  }
}

"And, given our less-than-glorious start, it would be a shame to end on anything less than this!":

private string CreateStringOfSelectedSupportedVersions()
{
  string res = "";
  int i;
  for (i = 0; i < m_VersionArray.Length; i++)
  {
    if (GetVersionEnabled(i))
    {
      if (res != "")
      {
        res += "|";
      }
      res += m_VersionArray[i];
    }
  }
  return res;
}

"Yeah," Brian sighed, "let's put it all back into a single string. That makes it so much easier to work with."

Something had to be done.

We Have a Problem...
Not wanting to perpetuate such a tragedy of code, Brian brought the code to the attention of both the lead architect and supervisor. Soon after digging through a bunch of Bert's other commits, bug reports, and some long-ignored code metrics, they figured that Bert was solely responsible for approximately 55 percent to 60 percent of the found bugs, and around 35 percent of the total added lines of code since he started. Not bad for an 18-man team.

The code that Brian worked against rang in a whopping 1,600-plus lines of code alone, not counting the UI-related code or any support functions that Bert created for this monstrosity.

It appeared that Bert talked the talk, but when it came to walking, he preferred crawling...in reverse...off a cliff.

Two weeks later, Bert was let go.