Skip to main content

Your code smells? Let it!

Code smell is a term used pretty commonly in software development.  It is a way for one developer to express to another that they're experience is telling them something is wrong, even though they don't see it on the surface.  In fact, that's what it's supposed to mean.

But software developers don't do well with "trust your gut" as a methodology, so we make an effort to identify and quantify things like this.  For that reason, over time we have managed to put together lists of things in your code that are "code smells" and explain why we don't like them.

For illustration, I'm going to refer to personal experience and talk about an application I'm currently working on.  A common code-smell that developers will talk about is singletons.  So that will be the basis for my example, but the message should be clear even without the example.

Why does it smell?

The first thing you should ask the developer on your team who's most knowledgeable about the code in question (that may be yourself) when you encounter a code smell is "Why does it smell?".  This is the part I see skipped--A LOT.  This is the most important part of the process.  Let's face it.  There are legitimate reasons for things to smell.  Your mechanic's shop should not smell the same as a bank vault!  Each code base is going to have its own set of smells, and that's ok, as long as the answer to "Why does it smell?" is reasonable.

In a project I've stepped into, singletons are everywhere.  I asked one of the developers about them and his answer was one sentence.  "We have memory constraints."  Oh!  That makes sense.  This application lives on (crappy) webservice connections, and is written in an older language, so it's logical to store those connection objects for reuse later, rather than waiting on a crappy garbage collector to delete them and running out of memory.  This code base smells like they're using singletons as global objects.. Guess what?  They are!  And it's OK.  That was their intent!

Of course, the answer is not always valid.  If the answer is "That's the way it's done" or something else vague like that, it's time to investigate yourself.  It may be that there is a legitimate reason and you haven't encountered it yet, but this is a code smell, and so you need to find the source.  You cannot, and should not go changing code to "fix the smell" if you don't know why it smells.

In my example, if I had started getting rid of singletons to "fix it" and making them instances of the service all throughout the application, I would have surely crashed production!  Don't go fixing things that you don't KNOW are broken.  Code smells are just that: a smell.  Some smells are supposed to happen!

What if it's bad?

So you've identified a code smell.  You asked "Why does it smell?" and you figured out, through some form of investigation, that it's bad.  This is the smell of burning rubber over a box of wires.  It's a bad smell, we need to fix it.

You're not supposed to fix the smell!

So many developers get this bit wrong.  The smell is your marker.  It identifies that there's a problem in your code base.  The smell itself is NOT the problem!  99% of code smells are pointers to an architectural problem!  The smell "goes away" when you fix the root problem.  Saying "x is a code smell" is not the same as saying "x is bad".  The code you're looking at solved a problem.  It is good code.  It just may not need to exist if you fix the part of the code that is bad.  The part that caused the smell to begin with.

As an example, in my current day job, they identified the singletons but knew that "singletons are a code smell".  They made the standard mistake.  They think that the word code smell means "bad".  So they say "singletons are bad".   Then they try to remove the singleton.  This actually happened in this code base.  They did something I'm seeing a lot on blogs.  They wrapped their singletons in a Dependency Injection library and pretended that, since the instance is being passed to their class via dependency injection, it's no longer a singleton.

Don't hide the smell!

Ok, so the new guy is just starting.  He didn't do any of the above, but he saw the singleton pattern and got all excited to help and swapped it out for dependency injection...  What did he do?  He masked the smell.  The smell is there for a reason.  It's like covering up the dog turd in the middle of the floor by putting a towel over it.  Whether this is a kennel, where that dog turd needs to be kept in the dog-turd specific area (maybe a singleton resolver class or some such), or a perfume store, where it needs to be cleared out immediately at all costs, he has made it harder to fix, and hard to identify for everyone.

In a code-base where the smell identifies an actual problem, to fix the smell masks the real problem and keeps you from finding it longer.

In a code-base where the smell points to outside forces beyond our control, the fix makes it harder to maintain, and harder for new developers to step in and learn about the code base.

Never, ever hide the smell.

What about your singleton problem?

With a big problem in an existing code-base like this, I'm starting small.  First thing to do, unmask all the singletons.  I'm creating a group of static classes that wrap up the singletons in the application.  Some day in the future, I'll be able to remove the Dependency Injection framework entirely and start over, using it correctly (for actual dependency injection).  But until then, I've had to think differently.  For now, in this particular code-base, whenever I see that dependency injection is being used.  That, for me, is a code smell.


Popular posts from this blog

Teams and Complexity

Let's pretend you're a car mechanic (I don't know, maybe you are).  But you don't work at some shop in town, you work at a bigtime auto-maker onsite at the factory.  You help build cars from scratch.  You work with a lot of other car mechanics, and various engineers of different levels.  If we were looking at the 1910s, you might be working for Henry Ford on one of the first ever assembly lines.  Either way, you have a specific part of the car you're responsible for.  Your skills probably extend beyond that, and you might work on a different part of the car each day as your manager moves you around to keep the factory efficient.

One of your coworkers, Bob, is a superb engineer.  He is very good at building cars, far better than you, and he does it faster than everyone else.  Your boss really likes him.  You often get stuck after him in the assembly line, so you know exactly what sorts of things he does.  He's not sloppy, but he likes to do things his way.  He w…

Managing Programmers

Working with other programmers is tricky.  That said, it's nothing compared to the job of managing programmers.  One of my favorite quotes about Perl is that (paraphrased) "a Perl developer is like a rockstar.  Now imaging having a bunch of rockstars in one room together and you will understand why you don't want an entire team of Perl developers."  It's not about Perl here though. What's important to understand is that any developer worth his salt is going to be like a rockstar.  And yes, there are a lot of professional developers out there who aren't worth their salt, but that's for another post another day.  Rockstar may not be the right term here, but think of it this way.  These guys are smart.  They may not be geniuses, but there's going to be things that they know that you don't and probably never will.

I've seen it more than once and it's not going to make some Product Managers happy, but I'm going to state a fact, an eleph…

How to identify a skilled programmer during an interview

How does one identify a skilled programmer?  No company that has interviewed me could tell the difference between myself and other programmers they'd interview.  The interview process is truly a game of luck in this industry--on both sides.  Both the programmer and the company are basing their actions entirely on luck.

Companies have come up with numerous methods to attempt to discern a good programmer from a bad one.  The best tricks they have include a series of math problems, algorithms, problem solving technique tests, and even obscure programming questions, some without definitive answers.  As an example: Is there an authoritative source of information on the core principles that define object oriented programming?  I've heard everywhere from 3 to 7.  In a field of research about a synthetic concept, an authoritative answer is almost impossible to obtain.

Programmers were then forced to study to the interview.  Careercup is one of my favorite sites for this.  This almost …