The more I work with deep inheritance hierarchies, the more I wonder if inheritance is actually a good idea. If you’re looking to avoid the trouble overusing inheritance can get you into, this article has some handy tips: Inheritance is evil. Stop using it.
We all know we have personal biases, but have you ever thought about your software design biases? I certainly didn’t until I read this article about Software Design Bias by Sandro Mancuso. If you’re having trouble with the design of a piece of software, it’s worth taking a look at Sandro’s handy list of biases and thinking about whether the way you prefer to design software is actually the best one for the particular thing you’re trying to build.
Today’s code smell is temporary fields. Like with all code smells, sometimes these aren’t a problem at all. For example, it’s pretty normal to have a user object with a bunch of optional fields. Sometimes you have users who just start out with a username and a password and fill in their profiles later and sometimes you have fields for stuff like social media accounts that only some users will ever fill in, but sometimes fields that only sometimes have a value are a problem.
One of the ways people try to fix the problem of a long parameter list is to make some of those parameters into fields and then set them later if they need to. Unfortunately that’s not really any better because it just hides some of the confusion under the rug instead of actually getting rid of it. Instead of it at least being obvious you have too many parameters, now you have a bunch of fields that may or may not get used depending on what the object is up to and you have to go hunting to figure out what’s going on.
Since professional programmers read code much more often than we write it, it’s a huge waste of time to have to go on an exploratory mission every time you need to understand that code again. If you do end up needing to update that code, best of luck figuring out which fields are important and whether your refactor / bug fix / new feature broke anything.
I had a heck of a time finding a code example for this code smell but fortunately so did Mark Seemann, who was frustrated enough by that to write not just an example of the smell but of how to refactor it too.
If you find this smell in your code, what do you do about it?
Pulling out the temporary bits into their own object and just using that is a good option (if that works for your code, of course). Isolating the fiddly bit makes the rest of the object cleaner and makes it clearer what the fiddly bit is up to. You can also add a null object (a special object where you put whatever defaults you use when one of your temporary fields is null). That way your normal processing code can be clean and simple instead of littered with null checks. Sometimes that just makes things more complicated, so it’s a bit of a judgement call.
If you’re having trouble figuring out what an object even does because most of its fields are null most of the time, see if some refactoring helps. You can always put it back the way it was if it’s worse afterwards :)
So I was browsing workplace.stackexchange.com questions late one night, as you do, and came across an interesting one: How to explain business priorities to a programmer.
That one didn’t go at all the way I thought from the title. It turns out the business priorities actually did make sense (the business had this wild idea that queries, even especially complicated ones, should take less than 4 hours to run) and the programmer was being unreasonably rigid in following “best practices”.
But weirdly, one of the answers said that both parties were wrong – that the programmer shouldn’t have been so attached to having nice looking code and the manager shouldn’t have insisted on using an anti-pattern. What? The anti-pattern (using raw SQL instead of the ORM) improved performance by multiple orders of magnitude! If performance matters at all, you cannot possibly tell me that massively improving performance was the wrong thing to do.
Okay, there is some nuance there :) If your ORM performs that badly it’s likely not configured right for your situation, so it might be better in the long term to fix your config than to keep writing custom SQL for every query that’s the least bit complicated. On the other hand, if you don’t have an expert handy who can tune your ORM config, it might be easier to ditch it, at least for the more complicated queries, and just write some SQL. Or hey, how about a compromise? Use a stored procedure, that’s what they’re for! If it’s a query you run a lot, it might even make sense to make a view for it.
No matter what you end up doing, it’s still not wrong to use a supposed “anti-pattern” if you have a good reason to do it. It’s the context that makes something a bad idea, even the best idea can turn bad if you use it the wrong way. The only way to know whether your “anti-pattern” is actually a bad idea for your particular situation is…. to look at your particular situation! You can’t just say something is an anti-pattern without knowing what the person using that anti-pattern is trying to accomplish.
Let’s look at another datebasey example. It’s generally considered a bad idea to denormalize your database, every database design course will tell you so. Normalizing your database ensures all of your data is always right by isolating everything that can change independently. Since you don’t usually bother to keep data at all if you don’t care if it’s all accurate, you almost always want your data nicely normalized.
But sometimes correctness isn’t the absolute most important thing. Let’s take browser or mobile games, for instance. If your game takes too long to load, your players will just go do something else and they might not ever come back. It’s much more important to be fast than it is to be perfect if you want to make money on a game like that. And if something does go wrong and a player gets mad because their data didn’t get saved correctly, you can just give them game currency until they’re happy again.
In a situation where performance is more important than correctness, normalization is the anti-pattern. Like I said, it’s all about context!
To be clear, there are some things that are just a terrible idea no matter what you’re doing. I don’t care how clever you think you are, your variable names need to make sense. And no matter how well it performs, if your system is so confusing than no one can make updates, then it’s a bad system. But for the most part whether or not something is an anti-pattern is about what you’re trying to accomplish more than it’s about the “anti-pattern” itself.
Everybody likes code that makes sense to them, and everybody likes to feel like they’re doing things the right way, but rigidly following “best practices” just isn’t good enough. If it doesn’t help you achieve your greater goals for your application (ie not just gold-plating your code, but providing value to your users or customers), it’s just not a good idea no matter how many experts call it a best practice.
And yes, that means you can’t know if your code is good or bad without knowing exactly what you’re trying to do. Welcome to professional programming!
Today’s code smell is long parameter lists. If you have lots and lots of parameters, something is likely going wrong.
Odds are good your method is just doing too much, which leads to confusing code that’s hard to understand and change. Another reason lots of parameters causes problems is that when you call that method, it’s very easy to accidentally get two parameters of the same type in the wrong order (not that that’s ever happened to me or anything).
How do you fix it?
Well, if you can break your method into separate methods that each do just one thing, do that. Not only does that solve the problem of a long, confusing, easy to mess up list of parameters, it also fixes the related problem of your method being confusing because it does too many things.
Another option if you can’t break up your method is to make a new object that holds all the parameters. Having named fields in the new object makes it a lot easier to keep track of which value goes where. If you already have an object that most or all of the parameters come from, it’s often simpler just to pass that along instead of pulling values out and passing those into the method you’re calling.
However, the more objects your caller and callee have in common, the more closely coupled they are. If your classes aren’t already closely coupled you’re probably fine to add another object they both use, but if they already have a lot in common then you should think about whether you really need that object. Of course, if your classes are extremely closely related they should maybe just be one class, but that’s a separate code smell.
And if breaking the big method into smaller ones means that the smaller ones absolutely always have to be called together, it’s probably better to leave it as one big method with lots of parameters. If you have to just remember what order to call the smaller methods in you’re asking for trouble, you will definitely mess that up sooner or later. A huge part of being a good programmer is accepting that it’s very very easy to mess it up and trying to avoid the most obvious pitfalls.
Like the other code smells, long parameter lists aren’t an absolute certainty you need to change your code, but I personally think they’re an especially strong sign that your design could be better. It’s just so easy to accidentally switch parameters, and even if you manage to keep them all straight it’s still a serious hassle to call a method with a huge list of parameters.
You know your codebase better than I do, though. Do what makes sense for your particular situation and don’t worry too much about my personal pet peeves :)
Today’s code smell is feature envy. Feature envy is when one class uses the methods of another class to excess. It’s called feature envy because classes that use methods from another class excessively look envious of that class’s features.
The problem with that, as usual, is the way your logic gets spread around between multiple classes. If you have one method that’s using another class so much that it’s hardly even separate from it, you’ll probably have to change things in both places when you need to fix a bug or add a new feature, but the compiler can’t tell you that you need to do that so you risk creating a shiny new bug by missing a spot.
Feature envy probably makes more sense with an example. Let’s say you have an address class and a customer class, and when you call customer.getMailingLabel() your customer class calls address.getStreetNumber(), address.getStreetName(), address.getCity() and so on to build a complete address string for printing a mailing label. Let’s imagine that policy changed and you’re now able to ship to rural mailboxes, so you need to add route and box numbers to your address and to use those instead of street number and street name if they’re present. If your code has feature envy, you have to change both the address object and the getMailingLabel method to support rural mailboxes. If you moved the getMailingLabel method into the address class, then you would only have to change one class when you wanted to change how you build mailing labels.
As with just about all code smells, feature envy isn’t always a sign there’s a problem. Sometimes you want to separate your data from operations on it, especially if you’re using a strategy or visitor design pattern. If, for example, you want addresses formatted differently for different reports, the formatting is probably more the job of the reporting code than the address object. In that case it would make more sense for a visitor to contain the formatting code so the address object doesn’t have to care about all the different reports.
Jessica Kerr has a really interesting blog post about how code reuse is often overrated. Back in college I learned all about how important code reuse was and how duplicated code was the worst, but we never (probably because we had a lot of material to cover in a limited time) talked about how code reuse can cause problems. Jessica gives a great explanation of how bad code reuse can paint you into a corner, you should check it out.
Today’s code smell is switch statements. Long switch statements or complicated if statements often (but not always!) mean that something has gone wrong with your design. Objected oriented languages give you a lot of tools to avoid big complicated switches, if you aren’t using one of those there needs to be a good reason for it. If you’re not using an object oriented language, on the other hand, I’m honestly not sure if switch statements are a sign of a potential issue. If any of my readers could weigh in on that, that would be really helpful.
But why are switch statements a problem? Because you’re spreading around logic that should probably be in one place, and if you add a value to one switch statement you’re likely to need to hunt down all the other ones and change them too.
If, for example, you have a switch statement that does different calculations based on the type of the object passed in, that logic for how to do the calculation probably belongs inside the object passed in to the switch. If you change that object, you’ll have to hunt down every place it’s used and make sure nothing else needs to be changed, which is obviously asking for trouble :) If you give each object a doCalculation method and replace the switch statement with a simple call to doCalculation, then all of the logic for that object stays inside the object. With all your logic in one place, you don’t have to search for every thing you need to change and risk missing a spot, and your design just makes more sense when logic for one thing isn’t smeared across multiple objects.
Polymorphism isn’t the only way to fix a switchy smell, if your method switches on a parameter that’s passed in, you may want to change it to separate named methods to more directly do the thing. If you have a setProperty(String name, String value) method, you could replace it with named methods like setType, setModel, setYear, etc. You might end up with a lot of little methods, but it’s still clearer than one setProperty method with a giant switch. If you end up with a truly excessive number of methods, that’s another code smell that points to your object doing too much.
Just because switch statements are sometimes a problem doesn’t mean they always are. Sometimes what you’re doing is so simple that an interface and a set of objects that implement it (or changes to your existing interface and objects) makes your code more complicated rather than less. And sometimes there just isn’t a better way to do it: factory methods and abstract factories rely on switches or a series of ifs to figure out which implementation to return.