Your code smells!!!

August 11, 2008 at 12:58 am 4 comments

I was planning to talk about Dependency Injection or the difference between Mocks, Stubs and Fakes, but I think I would prefer to get this one out first. For those of you who haven’t heard the term before, a code smell is something that indicates there is something wrong with the source code, be they design problems or signs that the code needs refactoring. So in this post, I would like to mention a few common code smells, their identifying patterns and how to fix them. So without any further delay, lets start :

  1. Too many comments
    The Problem : Lets start with the easiest to identify code smell. Comments are good, if they are describing a class or a method. But when you start having comments in your code explaining what a particular block of code does, you know you are in trouble.
    The Fix : If there is a block of code which does some complicated stuff, and you feel you need to comment it to make it easily understood, pull it out into a well named method, and let the method name describe what it does.
    .
  2. Long Class / Method
    The Problem :
    These two code smells actually are similar. If you have a method which goes beyond 10 – 15 lines, then you have a long method. While there is nothing technically wrong with long methods, its not as easy to comprehend as a nice small method, and can make maintenance a pain in the rear. Long classes also are similar, having too many things that its trying to do. If you generally try describing a class and have to use ands and ors, then you have a long class.
    The Fix : Pull out parts of the long method into smaller well named methods. The advantage is two fold. One, your method is much more readable now. And two, you can now test individual methods you have pulled out, making testing a much easier task than one giant method. Same with classes, break it up into multiple smaller easily testable classes.
    .
  3. Primitive Obsession
    The Problem :
    How many times have you had to write a class which takes in, say a phone number ? And how many times have you passed in a String or a long to the class which asks for the phone number ? If you raised your hand, then congrats, you have a code smell known as primitive obsession. This happens when instead of creating an object, you pass around primitives, and write functions to operate and convert that primitive from one form to another. So you might have to create utility classes which give you a phone number with brackets from a string, and so on and so forth.
    The Fix : Just give the poor thing a class. If you operate on phone numbers, create a PhoneNumber class which has methods to operate on the number. Makes it easier for anyone using the class as well, and of course, its testable :D.
    .
  4. Feature Envy / Inappropriate Intimacy
    The Problem :
    Feature Envy is when a method on a class is more interested in other classes than the class to which it belongs. The reason for this could be as simple as the method being in the wrong class to something more non trivial. Inappropriate intimacy is when two classes are so tightly coupled that one depends on the other to work a particular way for it to work.
    The Fix : For feature envy, just move the method to where it belongs. If it does work solely on some other class, then maybe it belongs on that class instead of where it is currently. For inappropriate intimacy, you need to figure out if the problem is something simple or something more complicated. It might be that the interfaces weren’t selected appropriately, or you might need to introduce a layer to keep the coupling loose, or even refactor the code to make sure it does not depend on another tightly coupled class.
    .
  5. Lazy class / Dead code / Duplicate code / Speculative Generality
    The Problem
    : All of the above are usually simple code smells which indicate you have code which you don’t need. Lazy class is when a class doesn’t do enough to justify its existence. Dead code is obvious, code which is not used or dead. Duplicate code, duh, Its duplicated. Speculative generality is the most interesting of the lot. Its when you write some code for something which you don’t need yet, but may need at some point in the future.
    The Fix : For the first three code smells, the fix is trivial. Delete it. Dont even think about it, just blindly delete it. Duplicate code is a pain to maintain as well, pull it out into a method and then delete the duplicates. Speculative generality is one thing people don’t realize they are doing or feel they need to do it now since otherwise, it might become difficult to do in the future. The interesting thing is that the feature they added speculatively is rarely ever used. Its an additional overhead to maintain and test for something you never use. Don’t do it. If you can implement it now, you can implement it when you need it. Just delete that darn code.

There are a lot more code smells than I could list out here, but these are a few of the most common ones you should keep a lookout for. Google search Code smells if you want to learn more about these insidious creations :D. Next time, before I start dependency injection, I think I will rant about things which make code untestable. So in a sense, Testability code smells.

Advertisements

Entry filed under: code, code smells, java, pinpoint, qa, refactoring, testability, testing, Uncategorized. Tags: , , , .

Mock, Mock! Who’s there ? Fakes and Mocks and Stubs, Oh My!!!

4 Comments Add your own

  • 1. Michael L  |  August 21, 2008 at 7:30 pm

    I dissagree with your first “code smell.” In my experience most developers dont have enough comments in code, (I am guilty of this myself at times). I think good commenting and good class names go hand in hand. It is also usefull because many times you have to turn over source code to other developers and if you have comments that out line the design or logic of a block of code, this helps other people down the line and yourself if you have to look at code you wrote years ago but forgot what it does.

    Reply
  • 2. shyamseshadri  |  August 21, 2008 at 9:34 pm

    I don’t say, do away with comments. I agree, especially class level and method level comments are essential in outlining what the code is doing. What I am saying though, is if you need to comment what a particular block of code is doing, your code is not self explanatory and could probably do with good naming. Also, pulling it out into a separate method of its own with good documentation on the method level is probably aeons better than writing some comments inside the code there itself. For one, it allows you to reuse it, and two, it allows you to test that snippet in separate.

    For example, this is what I meant by too many comments is a code smell :

    double getProjectSkipAverage(String projectName) {
    List data = new ArrayList();
    // If local cache is initialized and it contains the project info
    if (localcache != null && localcache.contains(projectName) {
    data = localcache.get(projectName).getData();
    }
    // If not enough results, get it from the database
    if (data.length() < 5) {
    Datastore datastore = DatastoreFactory.getInstance();
    data = datastore.query("Name=" + projectName).getResults();
    }
    // If still no results, return 0
    if (data.length() == 0) {
    return 0;
    }

    // Otherwise, do a skipping sum, skipping each alternate value
    int sum = 0;
    for(int i = 0; i < data.length; i+= 2) {
    sum += data.get(i);
    }

    // return the average
    return sum / data.length();
    }

    Whereas you could do this instead :

    // Documentation here
    double getSkipAverage(List data) {
    if (data.length() == 0) {
    return 0;
    }

    int sum = 0;
    for(int i = 0; i < data.length; i+= 2) {
    sum += data.get(i);
    }

    return sum / data.length();
    }

    List getDataFromCache(Cache localcache,
    String projectName) {
    if (localcache != null && localcache.contains(projectName) {
    return localcache.get(projectName).getData();
    }
    return new List();
    }

    List getDataFromDatastore(String projectName) {
    Datastore datastore = DatastoreFactory.getInstance();
    return datastore.query("Name=" + projectName).getResults();
    }

    double getSkipAverage(String projectName) {
    List data = getDataFromCache(localcache,
    projectName);

    if (data.length < 5) {
    data = getDataFromDatastore(projectName);
    }

    return getSkipAverage(data);
    }

    Now each of these methods can and should have documentation at the method level, and each of them are nice small easy to test methods in and by itself. This is what I prefer, any day.

    Reply
  • 3. Michael L  |  August 21, 2008 at 2:55 pm

    Well I usually do not comment code that is strait forward and easy to user stand. Now that I think about it, point one is saying if you code is so complicated that you need a lot of comments to explain what is going on, then this is bad sign since if you can do the same thing with more simple version that is better and improves maintainability of software. But saying “no comments in source code” is not a good idea ether because sometimes you have to go the complicated route. Also if I am looking a piece of code, sometimes the names of classes and functions make sense to the developers that write them, but when you show them to someone else might make no sense at all. In fact I do many code reviews, and sometimes the names make no sense to me, since they have acronyms in the names. So comments in that case would help a lot to a person who does not know those acronyms.

    Reply
  • 4. shyamseshadri  |  August 22, 2008 at 1:20 pm

    Oh, I don’t say, no comments in source code. Again, Class level and method level comments are essential in any code base. All Point 1 says is if you need comments to explain the internal workings of any method, you are probably doing something wrong. Again, this is not a hard fast rule, but rather a guidance and something to watch out for if this seems to be happening more often than not.

    Reply

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Trackback this post  |  Subscribe to the comments via RSS Feed


OmniscientOne

Blog Stats

  • 5,594 hits

Feeds

Pages

August 2008
M T W T F S S
« Jul   Sep »
 123
45678910
11121314151617
18192021222324
25262728293031

%d bloggers like this: