DevDisasters

Fixing Bad Code from a Contractor

DevDisasters

Brandon hired a software development contractor to create a Web portal using ASP.NET to complete a custom software solution for one of his company's Software as a Service customers. As the project manager, Brandon had budgeted this portal for no more than four weeks of work. If the project were done in-house, utilizing the company's custom app framework, it would've taken no more than two weeks for the internal team's slowest developers. The dev group simply didn't have the resources to assign to the project, so Brandon needed to subcontract.

Brandon trained the independent contractor Mark on how to use the app framework. The dev team would code any server code, so Mark was only responsible for the ASP.NET portion of the project.


After about six weeks, Brandon started to realize that things demonstrated to the team as "Complete" weren't working properly once the developers got their hands on them and were close to UAT deployment. Features were semi-functional -- meaning highly unstable -- and it was clear that Mark's nine-year resume and reputation touted by the placement agency were completely farcical.

Good Cop, Bad Cop
Regardless, as the project manager, Brandon got more involved with Mark's code, and so did the company's CTO. The CTO and Brandon decided to do a "good cop, bad cop" routine on him. The CTO, the bad cop, pressured Mark into delivering as he promised or the company wouldn't be able to afford his services.

Brandon got a phone call around 8 p.m. on a Sunday night. The team was supposed to demo the portal for the client the next morning. Mark wanted Brandon to look at his code because he had an "issue."

Mark explained that the data coming from the server wasn't matching the data on the front-end. He stated that the data from the server was correct. (And of course it was -- Brandon's team wrote that code for him.) Brandon asked Mark how he was merging the data together and he said that he was using the "merge" function. Brandon logged on and looked at his code and was completely shocked at what he saw!

Simple Task
Mark actually wrote more than 240 lines of code to manually merge data from two datasets. Brandon had to mute his microphone to call his wife over to see how pathetic this guy's code was:

btnSave_Click Method was 42 lines; Brandon changed it to four lines.

The method SetCaseRow was 205 lines of completely useless code. Brandon deleted it and replaced the call to SetCaseRow with the DataSet.Merge function:

  // Merge with the edited data
  casedata.Data.Merge(
    RefreshCasedatafromInputScreen(
    (CASEDATA)Session["UpdatedCaseData"]
    ).Data);

RefreshCasedataFromInputScreen was 120 lines of code; Brandon called an already existing function, saving another 120 lines.

All in all, he shrunk 363 lines of code (including the more than 240 lines of merge code) into three lines in about 45 seconds. Brandon shook his head as he explained to his wife that Mark had probably spent two or three days writing code -- which didn't even work properly -- when there's a built-in function included by Microsoft. The worst part: Brandon's team had shown Mark some examples of how to merge the data from their server into his UI.

Tell Us Your Tale

Have you experienced the darker side of development? We want to publish your story. E-mail your tale to Executive Editor Kathleen Richards at krichards@1105media.com and put "DevDisasters" as the subject line.

Merge Disfunction
Brandon tried not to imagine what would have happened if Mark had tried to implement this type of craziness with all of the schemas and datasets they used in the project. The built-in DataSet.Merge function works with any dataset, while Mark's function worked only with the specific type of dataset for which he had coded. He could have spent weeks or months warping his brain to write merge code -- hundreds of lines for each different type of dataset. Had he only known about the merge function!

Mark finished by telling Brandon that he was so thankful Brandon could help him clean the code up because he was afraid the CTO would see it and tell him there was a better way. Brandon had to tactfully tell Mark that his code was the worst code -- and dumbest thing -- he'd seen in years. Mark was let go that week and Brandon's team had to build the portal in-house.


About the Author

Kathleen Richards is the editor of RedDevNews.com and executive editor of Visual Studio Magazine.

Reader Comments:

Fri, Oct 15, 2010 Brandon USA

Good Comments. We use datasets because we have a mature framework that uses them and we have successfully developed many solutions on this framework. We are dying to move to a pure OOD, but we don't have the resources to make a new framework and then update our existing software. We all know that we don't live in an "ideal" world, so stop pretending like anyone ever looks at their work and doesn't see how it could be redesigned to be infinitely better. The real issue here is that the contractor was given examples of how to use our API and framework. He used them and demonstrated the application multiple times. The trouble arose when he decided to get creative to solve a problem that wouldn't have been a problem had he asked. We train our contractors and have code review. In this case the contractor was good enough to code a box around him, just not good enough to code himself out of the box.

Fri, Oct 15, 2010 Frank

Wow so much hate towards datasets and the code nobody here knows about... It's just another tool in the drawer. Software is not religion, but people tend to treat it as such. Learn to use the tools at your disposal to accomplish a task. In the end if the product is helpful and maintainable you have done the job correctly. But that's the difference between fanatics and mainstream with code and religion.

Fri, Oct 15, 2010 Billy NJ

On the fixed code why use a session as a parameter without checking if it still has a value? An error waiting to happen

Wed, Oct 13, 2010 Anonymous

The first mistake is using DataSets. Aren't we past that pile of shit yet? So tired of seeing DataSets still. That is just nasty pile of code to begin with no matter how clean the rest is...DataSets are amateur....People get out of this DataSet nightmare please!

Wed, Oct 13, 2010

Even the "fixed" code has problems. 1) Using "dd" for a variable name = bad 2) Using underscores to begin a variable name = bad 3) Not using camelCasing for variable names and not using Pascal casing for method names = not up to .Net standards 4) Assuming that the session variable is going to have a value without checking for null = bad 5) Assuming that the Session value is going to be Int64 compatible = bad (should use TryParse) 6) Calling a method and directly passing as a parameter = OK, but it's better to first assign the method result as a variable and then pass to another method

Wed, Oct 13, 2010 Dan Dallas

Hiring good developers is challenging. Whether the developers are contractors on in-house makes no difference.

Wed, Oct 13, 2010 Allen Florida

Why would anyone use DataSets at the UI layer anyways in a layered application? I think the developers on both sides should be let go and fresh minds should be brough in that know something about OOD.

Sun, Oct 10, 2010

There's a very good lesson here, simply not to duplicate what is in the framework. I was struck by how much Brad left Mark on his own. Also, having trained Mark previously I would have expected him to have a better handle on what Mark was able to accomplish before allowing him to work on this project. I'm a relatively new developer and sometimes I do what Mark did, other times I get it right. I understand biting off more than you can chew and loosing perspective on the project. Sometimes you grow from it, other times your ship sinks. I've noticed VS Magazine writing the Dev Disasters from the perspective of a developer with experience. From a n00bs perspective I find the article arrogant and evidence of companies and experienced developers lack of leadership and mentor-ship to those around them.

Fri, Oct 8, 2010 contractor Kansas City

I usually experience the opposite. As a contractor, I find a lot of code libraries full of methods that duplicate functionality already in the .Net framework. It's usually my job to fix code somebody inhouse created. Both contractors and inhouse developers need good oversite, clear objectives, documented code-standards and code reviews.

Fri, Oct 8, 2010 Ron

Unfortunately, this happens all too often. At my company, I constantly do this for the contractor code that we get. I keep wondering why we pay these guys anything.

Fri, Oct 8, 2010 Mark MN

I never received payment for all those lines of code!

Fri, Oct 8, 2010 Paul Minneapolis

Mark was foolish for not following directions and asking questions. Brandon was foolish for not having someone on his team work closely with Mark; daily code reviews would have fixed (or at least identified) the problem right away.

Wed, Oct 6, 2010 John New York

Alliance Global Services is the only regional software development firm that successfully combines onshore leadership with offshore value. Strong expertise in Product Development, Application Development Services, Custom Software Development, Application Testing Services and Web Application Development, it consulting boston. For More Details: http://www.allianceglobalservices.com/

Tue, Oct 5, 2010

Ask anyone who has worked with outsourcing about resume credibility...Vacuous!

Add Your Comment:

Your Name:(optional)
Your Email:(optional)
Your Location:(optional)
Comment:
Please type the letters/numbers you see above