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.
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!
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.
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.
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.
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.
Comments
Post a Comment