Code deodorant: practices to avoid code smells

Question

Excessive use of magic numbers or string literals in code is something of a code smell; not necessarily wrong but worth considering carefully. However, one can set up your editor/IDE to highlight string and number literals in garish colours like pink on lime green. That way, you can't help but notice where you are using these types and, hopefully, this practice will force you to consider carefully how you structure your code and perhaps avoiding the code smell, or concentrating it in well-defined places (like declaring all string literals as constants). Code deodorant if you like.

Another example practice is to use a large font size in your editor. This means that a smaller number of lines of code are visible on the screen which makes writing excessively long methods and classes more difficult - effectively guiding you towards writing short, purposeful functions.

What are other development/management practices which steer your programming in the right direction and help avoid programming code smells? See http://stackoverflow.com/questions/114342/what-are-code-smells-what-is-the-best-way-to-correct-them for an excellent catalogue of smells.

Answer 1

What i've been doing happens at commit time. Before I commit a change, I go through the diffs, comparing before and after. I comment on each change in the commit message as i go along.

If I can't figure out why something is different well enough to explain it in the commit message, then it's not ready to be commited. I'm not talking about other peoples changes, either. It is always me that made the change.

Either I refactor that piece of code until it makes sense, or it just doesn't go into the commit.

Answer 2

Wouldn't code deodorant be analogous to making bad code smell good...ie disguising the problem? Also, isn't your suggestion to highlight the hard-coded values more code-oderant rather than deodorant?

To me, avoiding code smells is a matter of writing good code over time by refactoring refactoring refactoring. I don't think anyone gets it right the first time...but code smells usually result from code that no one's refactored after the original purpose of that code no longer meets the current requirements.

Answer 3

If you start seeing any of these, it's time to refactor:

• Hard coded values which should be stored in configuration instead.
• Try/Catch blocks used as logic
• Methods which have grown too large.
• Classes that do too little
• Classes that do too much
• Code that does not observe the DRY principle
• Improper encapsulation such as inappropriately publicly scoped methods and/or properties

Answer 4

I run Source Monitor regularly on my code. Every time I see a comlexity value greater than 8.0 I start refactoring.

Answer 5

I am not sure marking all string literals in garish colors will help - thnk of messages in non-user-visible exceptions and asserts. They are one-time strings, very useful and you would not want to refactor them into variables.

An approach that consistently works is self-discipline and code reviews. You should get that uncomfortable feeling on seeing literals spread around, esp. those used more than once.

Answer 6

A lot of the practices listed here also apply to removing code smells:

http://stackoverflow.com/questions/490420/favorite-clever-defensive-programming-best-practices

Answer 7

One thing I use to identify C&P code at a glance is to look for patterns in the code. Heck, even in the SHAPE of the code.

If you see any shapes repeating themselves in your code, you've probably got something that needs to be factored out.

if(var1 != null)
    class1.method();
if(var2 != null)
    class2.method();

That's enough of a pattern that it should look wrong to you (this example doesn't mean anything, just the fact that it's a pattern that repeats)

I've seen a LOT of code like this:

Btn1.addActionListener(new ActionListener() {
    action(ActionEvent e) {
       ......
       method1();
       ......
    }
});
Btn2.addActionListener(new ActionListener() {
    action(ActionEvent e) {
       ......
       method2();
       ......
    }
});

Where everything is identical except the method being called.

This type of repetition is really encouraged by anynomous inner classes, they are so common that programmers often forget that it can be better to create a full-fledged listener class.

In fact, in this case the listener should probably be a parent with two subclasses that forward the middle part to either method1 or method2 depending on which class was instantiated and added as a listener.

Answer 8

"Code smells" is concept pioneered by the Agile Community. I would suggest using rest of the agile approach along with this concept. Just about every programming pattern and every refactoring approach comes out of one or another code smells, see Martin Fowler's book, Refactoring. However, agile development using refactoring is about producing something that works now and then improving it over time rather producing pure code. So you will get problems in your code (Of course, every other method also also will get you problems too). The advantage of agile is that you refactor as you go, using your user stories as a guide for which way to go, so your code gets better despite never reaching perfection.

-- and you did write tests to allow you to refactor easily, did you not?.

Answer 9

Many descriptions I've seen for the process of using Code Smells is to first write code so it works, then refactor it based on the smells. In other words, don't spend a lot of time planning out your design anticipating smells and avoiding them (although obviously your habits and patterns will improve over time) but get something out there and then improve it.

Otherwise you run the risk of incurring the wrath of YAGNI.

---

For those few not familiar, Martin Fowler's seminal book, "Refactoring", starts out with an extensive enumeration of what he calls "Bad Smells", and then proceeds to explain refactoring in terms of deodorizing those smells.