Tuesday, April 26, 2005

Long Methods and Classes Are Evil

I've spent a great deal of time in the last week reverse engineering legacy VB6 code, and I'd like to rant a little bit. What's making my life harder is how large and complex the methods are in each class module. The code is very procedural, with little usage of subroutines. The VB functions are several pages long, if/then nesting goes 6 or 7 deep, and case statements span hundreds of lines. Unsurprisingly, the code is hard to maintain and brittle in regression testing. Finding the location of a piece of functionality requires arcane knowledge of the application or some amount of luck with CTRL-F.

If the application you are building or modifying is important or useful, it is a sure bet the code will have to be modified later as requirements change and problems occur. There are some easy steps to take to make your code more understandable. Good naming is obviously important, a variable named "a1" isn't very descriptive, especially when it is an untyped variant. Can you say "Mystery Meat?"

My personal pet peeve is really long methods and classes that do way too much. I'm a terrible slob in most other ways, but I want my code readily organized with each type of code on its proper shelf, err, class. The human brain can only process so much information at a time. Small methods can be seen and understood all at one time. Bugs can hide out in very long methods because there is simply more brush to hide in. Large case statements are veritable camouflage for lurking bugs.

Rules of Thumb for Class and Method Size

  1. Any method should fit into your IDE window so the entire method can be read at one time. I'd take it farther if you can. One of the few code metrics I pay attention to is the average lines of code per method. I strive for an average of not more than 5 lines of code per method.
  2. Too many private members. You know what I mean, if you see a class with an excessive number of private fields (say >10), the class is probably too big. I'm tracing a class with about 75 private fields this morning. Not pleasant. In Fowler's Refactoring book this is described as the code smell "Primitive Obsession."
  3. The size of the code file. Offhand, I'd say any class over 10k in size is getting too large for its britches. A class that is 200k in the file system is certainly too long. TestFixture classes might be an exception.
  4. Exception handling code and instrumentation tend to push methods to be much larger. Invest some thought in how to segregate this type of code away from the main functionality. Aspect Oriented Programming may justify its existence by merely doing this.
  5. Reduce the number of public methods in a class. Just picking a number, I would say less than 10 in most cases. There is a good discussion on this subject at http://c2.com/cgi/wiki?FewShortMethodsPerClass.

I'm not just picking on VB developers, because I've seen (and written) bad examples in other languages, including a C# system that is featured in an early case study on MSDN as a shining example of the wonders of .NET. At a former employer, my teammates and I inherited a large new supply chain system hacked together by a very prominent consultancy. I distinctly remember a coworker printing out a Perl script that contained an if/then branch spanning 18 pages.

As an aside I'm 2-3 years removed from any kind of serious VB6 work. I remember a time when I was a staunch pro-VB guy because of its productivity for building code rapidly and hated being looked down on by Java guys. After becoming quite used to C# coding with ReSharper and other toys, VB6 feels extremely clunky. Combining well-factored C# code with the advanced code navigation features from VS.NET itself and ReSharper, leads to code that is readily traced and reverse-engineered. Oh yeah, I forgot to mention that variants suck and COM sucks more. I do still hate being looked down on by Java guys, though.


Anonymous Anonymous said...

I worked at one place doing bug fixes (a lead-in to a nice B2B project with BizTalk!) and the entire ordering process was in 1 ASP.NET page! Tt had 28,000 lines of code in the code-behind!

Darrell Norton

8:19 AM, April 27, 2005  
Anonymous Jeffrey Palermo said...

I feel your pain. Sometimes I have to go through legacy stored procedures that a huge! Back in the day when the Sql Xml craze was in full force. Suddenly, with SQL 2000, _every_ sproc had to take an Xml string, and then we'd OpenXml on it and use it. This allowed the passing of multiple records and some pretty scary T-SQL cursors. Yuk! Fodder for The WTF.

I don't know what the magic metric is, but I agree that good OO design will result in smaller classes with smaller methods. If a class exists to fulfill a single responsibility, it has to be small. If you _really_ need a single class with a bunch of methods to do a bunch of stuff, that's what facades are for. The facade would have a bunch of methods with about 3 lines each - delegating functionality to the owner of that responsibility. The facades responsibility is merely to delegate.

Darrell, don't get me started on code-behind :). So many people put _way_ too much code in the code-behind. The code-behind is the view, and the view should do nothing but manage its UI controls. All behavior should be delegated.

10:19 AM, April 27, 2005  
Anonymous Anonymous said...

I had a developer who insisted on writing stored procedures that where 1400+ lines which only hand one insert statement that did the job. The rest was PRINT statements which printed the same information over and over after a series of almost useless conditional logic statements. His justification to me was that they were "maintainable". It contained enough documentation and diagnostics for other developers to do their job.

Well, he doesn't work for me anymore, but I still have the sprocs.

I hope we can get rid of them by using NHibernate, but that remains to be seen.

I would also say my belief in Pair Programming or simple code reviews has benefited from this experience.

P.S. Wasn't one of the reasons I gave you this project was because the code was so un-maintainable. :)

6:28 PM, May 14, 2005  
Anonymous Anonymous said...

Don't worry about Java folks looking down on you. Do you really care what the least productive developers in the world think?

1:06 PM, December 10, 2006  

Post a Comment

<< Home