Brendan Enrick

Daily Software Development

Massive Multiplied Methods - Code Audits #5

I am coining a new term and have just googled to make sure that it’s not written anywhere yet. “Massive Multiplied Methods” did not appear in a Google search at all. I establish that I am likely the first person to post those 3 words together in that order on the Internet.

I am sure that everyone reading my blog knows that you should not repeat yourself and that your code should be organized in small cohesive parts.

Sometimes developers don’t do that. Occasionally, we can find places where people completely ignored those two.

The Bad

Imagine a site whose content comes from editors uploading lists of content stored in spreadsheets. Each spreadsheet, for some reason, has to have only 1 type of content (even though the data all has the same fields).

Now imagine that when the system reads each row from the spreadsheet, it needs to map each value onto a specific record from the database. The data is normalized, so what was a string in the excel file needs to link to the correct other table in the database.

The user selects which type of content is being uploaded and then the system will call the method for uploading that content. Yes, it’s just one method. Any other methods called are part of the .NET Framework. Making things worse, the methods are long. Very long. Each one was about 1,500 lines of code.

There were 5 different types. Each of these types has its own 1,500 line method. That’s about 7,500 lines of code for these.

And the best part about those 5 methods? They’re all identical except for 1 string. Literally, the only difference between these methods is the magic string literal used for the type.

The Better

I am not sure what to say here other than. Go learn about parameters in methods and how to create methods to break an algorithm into logical parts. Wow. Just. Wow.

It’s bad enough that the string could have just been in the spreadsheet to begin with, but did it really require 5 separate methods?

To top it off, you could have just accessed the value from the drop down list the user selected from to indicate the type. This was all in a code behind file anyway, so it could have just grabbed that value. (It shouldn’t have been there, but this would at least be slightly better.)

I don’t know how people let copy and paste operations get that bad. To be honest, I am amazed that the methods still matched. I guess people just avoided changing the code because it was too ugly. (That’s a mistake, because we need to clean up the scary code.)

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Massive Classes - Code Audits #3

Today we’re going to take a look at something that I’m sure most of you have seen before.

We’re talking about classes that have grown so large that they’ve earned the size category “massive”. These classes can sometimes house an entire layer of an application. Who would create these monstrosities? Usually it’s a team of malicious, evil developers who are trying to ruin your day. The devised a clever way to make your development experience terrible; they create a massive class and make sure that you end up having to maintain/repair/modify the code.

The real question everyone should be asking is “how big?”

The Bad

I could write all day long about different instances of these that I’ve seen, but instead of boring you, I will only talk about one of the worst offenders I’ve ever seen.

One fantastic code base we inherited was built using a “great” technique for organizing the code. It’s simple approach state that any time you needed to do any data access you could just go to the class named, “MasterDAL”. What made this so great? Well, you always knew where the method you needed would be. What is wrong with that? Well, it was quite common to have 2 or 3 methods that did the same thing in that massive class.

Sometimes you had a few methods that all did the same thing, but each one did it slightly different. The really fun part is that some of them worked and some of them didn’t. It was your job to make sure that you used one that had been updated and had its bugs fixed.

How big was MasterDAL you ask? That file alone was over 50,000 lines long. It was not auto-generated. It was written by developers.

OK, so something I told you so far about this wasn’t true. The code wasn’t really this nice. It gets better. I lied about there being 1 place for everything. MasterDAL was the master data access layer. There were two other data access layers.

Luckily those ones had much more specific names and were not as general or as large as the MasterDal. MiscellaneousDAL and OtherDAL were each only about 20,000 lines long. Obviously the miscellaneous data access and all the other data access go in those. That’s how they’re different from “Master”.

Oh and did I mention that not only did it have repeated code internally, but it also had some methods repeated in the MasterDAL. I know you’re surprised now.

The Better

Please always make sure that you’re organizing in smaller classes. Go read about the Single Responsibility Principle. It will explain that we need to organize our code. We also need to not repeat ourselves quite so much.

There are so many great ways of dealing with data access. Go get yourself an ORM or just use Entity Framework. I don’t care how you do it, but use one of those. Go read about some designs for organizing your data access code. There are so many good options out there. There’s no reason to create these big messes.

More Code Audit Nuggets

Keep watching for more interesting nuggets of stuff that I’ve seen in codebases.

Single Responsibility Principle

Now that we’re in the second half of June, I think it’s time that those of us with one, take a look our 2011 Software Craftsmanship Calendar. In June 2011, the topic was the Single Responsibility Principle, which states that your code should only have one reason to change.

The Single Responsibility Principle is the first in a set of Principles called SOLID,

SingleResponsibilityPrinciple

That looks dangerous…

For the rest of June, please try to follow Single Responsibility Principle and try to not be a Duct Tape Coder. Everyone who has to maintain your code in the future will thank you.