Code Smells – How to Find the Stench in Your Code, Issue Number 2

Welcome to the second edition of Code Smells! Periodically I’ll be posting about how to detect code smells and what they mean in terms of the big picture of your code. The previous installment can be found right here.

What’s a code smell? Wikipedia says it perfectly:

In computer programming, code smell is any symptom in the source code of a program that possibly indicates a deeper problem. Code smells are usually not bugs—they are not technically incorrect and don’t currently prevent the program from functioning. Instead, they indicate weaknesses in design that may be slowing down development or increasing the risk of bugs or failures in the future.

Onto the code smells!

The Stink List

Code Smell #4

(Thanks to reddit user fkaginstrom) You have an large number of parameters being passed into your function call. Functions that take in a ton of parameters stink for a few reasons. How many is too many though? This is a topic that people have debated all over The Internet. This Stack Overflow answer even quotes an author saying to never have more than three parameters in a function. In my opinion? There’s no fixed number. It’s going to vary from situation to situation, project to project, class to class, and method to method. Putting a fixed number on it is sort of setting up a rule to be broken.

What can you do to avoid this kind of smell? This C#-based Stack Overflow thread has a bunch of great ideas. One simple solution is just to bundle things into logical groupings of data. An example (although, it’s potentially a poor example since it’s only two parameters) is x and y coordinates. You can bundle these into a custom point type and pass this into functions. Now a function that may have taken four pairs of coordinates is reduced from eight parameters down to four. This approach also introduces the dependency on your custom type for your function, but I’m just offering it up as an option. If you’re always passing around the same group of X pieces of data around, it may make sense to bundle them into a single container type.

A side effect of reducing the number of parameters your functions require is readability. It might seem minimal, but having functions with only a handful of parameters keeps them from becoming unwieldy and much easier to understand when scanning through code. Readability is sometimes overlooked by developers, but when you’re in a team (and most developers work in teams), it goes a long way.

Code Smell #5

(Thanks to reddit user fkaginstrom) Your class has a large number of methods. If we keep the Single Responsibility Principle in mind (which states that a class should have one reason to change), it’s a warning sign that we might be creeping in on violating it. How? If more and more methods keep getting added, more responsibilities/capabilities can sneak in. This MSDN blog article also highlights some examples of the Single Responsibility Principle. Essentially, as the methods within your class grow in numbers, your class becomes responsible for more types of things. If you later on want to use  just one of those things in a different context, you’re now required to use one big heavy-weight type. Of course, this heavy-weight type comes with it’s own bundle of dependencies, setup requirements, and so on.

How do you avoid this? You can start by refactoring your monstrous type into multiple types. If your type has 12 methods that it defines, and they fall under three general categories of functionality, consider making three interfaces to group the functionality. Then you might consider adding three classes that stay true to these interfaces. The MSDN article I mentioned before does a good job of explaining how this kind of approach works.

Code Smell #6

Your single method has grown to hundreds of lines. This is one code smell I find that newer programmers introduce more frequently than experienced programmers. However, when you’re working on an enormous code base, sometimes this type of thing sneaks right up on you. So what’s the problem with having one method do a ton of things? It’s a convenience, isn’t it? let’s say someone only has to call one method that can launch a rocket, play golf, and invest in the stock market while filming a blockbuster movie. That’s power and ease of use, no?

This is related to Code Smell #5, in my opinion. The convenience of being able to call a method that does all sorts of fancy things at once is the exact inverse of the problem you face when you want to test the method. If I just want to test that I can successfully start the burners in the rocket, I have no choice but to call the method that does everything. What makes this problem even worse is that once your code has been structured this way, breaking down big methods into smaller methods can prove to be a challenge. When you see how dependencies are passed down the call hierarchy, or where certain classes have knowledge of others, things become scary.

I’ll give one real-life example of something I saw recently in a particular code base. A test had to be written to cover a problematic area of code that had been refactored. There needed to be some sort of verification in the code that proved this section was behaving as expected under particular conditions. Great stuff. Except the section of code that existed inside of a method that did the following:

• UI interaction
• Database read
• Data processing
• File read
• Data processing 2
• Database write
• External disk operation* (This one was pretty specific to the project I’m describing, but it wasn’t just a simple file read/write)
• File write
• UI interaction

Where the highlighted “Data processing 2” is the section of the method that needed testing. How’s that for fun? In order to test this one section properly it required refactoring of all of the encompassing code so that we could test it as a unit.

Have your own code smells? Share them in the comments. If you’re interested in more learning opportunities, subscribe to my free weekly newsletter and check out my YouTube channel!

Nick Cosentino Principal Software Engineering Manager

Principal Software Engineering Manager at Microsoft. Views are my own.

See Full Bio

ASP.NET C# DotNet .NET Software Engineering