Over a million developers have joined DZone.
{{announcement.body}}
{{announcement.title}}

Code Smells (Part I)

DZone's Guide to

Code Smells (Part I)

In the first part of this series on code smells, get an introduction to the various types of smells that can crop up in your code and learn how to deal with them.

· Java Zone
Free Resource

Are you joining the containers revolution? Start leveraging container management using Platform9's ultimate guide to Kubernetes deployment.

Last weekend, I was at SoCraTes Canaries and I gave my first talk ever about code smells. Oh boy, how nervous I was! But now that it has passed, I was wondering what I should do with all information I gathered. And then I thought, "Maybe it's a good idea to put it all in a nice blog post."

So, What Are Code Smells?

As Martin Fowler said in his book "Refactoring: Improving the Design of Existing Code",

A code smell is a surface indication that usually corresponds to a deeper problem in the system.

I like to think that a code smell is something that makes your developer instinct cry out to you, and you just know that something is wrong. This doesn’t mean you have to make changes in your code: there are occasions where these code smells are ok, but I think it’s important for us to detect them and know exactly why they are there.

There are five categories of code smells:

  • Bloaters
  • Object-Orientation Abusers
  • Change Preventers
  • Dispensables
  • Couplers

Today I'm going to talk about Bloaters. I'll leave the other categories for a future post.

Bloaters

Bloaters can be big methods or classes, primitive obsessions, data clumps, or long parameter lists.

Long Parameter List/Data Clumps

The Long Parameter List is when you have a method that has more than 3 parameters. Sometimes we see that when we receive an object, and instead of passing it all we pass some of its data. In this case, the best policy is to pass the whole object. Data Clumps are a bit different: they are, in general, primitive values that start to "get together". A good example of this is a startDate and endDate... Maybe it's worth creating a DateRange.

Primitive Obsession

This case is when we use primitives instead of value types for simple tasks. Sometimes the use of primitives is justifiable, but when you start to have behaviour attached to this primitives, then it's time to stop and think that maybe a value type is in order. A simple example is a currency: we tend to put it in a float or double, instead of encapsulating it in a value type.

Long Method/Large Class

This kind of code smell happens when you have a big method. But when do you know that a method has become too big? Well, I have the rule that with more than five lines, you should, at least, look at it again. But, as Sandro told me before, the right number of lines is just enough lines so a method only does one thing (and so it conforms to the 1st principle of SOLID the Single responsibility principle.

To do this blog I started to look at my old code when I hadn't woken up yet to craftsmanship: if it was working that was good enough for me. Here's the code in Objective-C:

- (void) postToServer
{
    PostSerializer* postSerializer = [[PostSerializer alloc] init];
    NSString *post = [postSerializer serializePostWithTitle:self.txtTitle.text description:self.txtDescription.text author:self.txtUser.text game:self.game];

    NSMutableDictionary *postParams = [NSMutableDictionary dictionary];
      [postParams setObject:txtTitle.text forKey:@"title"];
      [postParams setObject:post forKey:@"data"];
      [postParams setObject:txtUser.text forKey:@"username"];
    [postParams setObject:txtPassword.text forKey:@"password"];

      NSArray *args = [NSArray arrayWithObjects:[NSNumber numberWithInt:0], postParams, nil];

#ifdef DEBUG_LOG
    XMLRPCRequest *request = [[XMLRPCRequest alloc] initWithURL:
                              [NSURL URLWithString:@"http://localhost:8888/letsbasket/xmlrpc.php"]];
    DLog(@"Debug");
#else
    XMLRPCRequest *request = [[XMLRPCRequest alloc] initWithURL:[NSURL URLWithString:[UtilsHelper localizeString:@"UrlXmlRPCKey"]]];
    DLog(@"Producao");
#endif

      [request setMethod:@"letsBasket.AddPost" withParameters:args];

    NSError *error = nil;
      XMLRPCResponse* result = [XMLRPCConnection sendSynchronousXMLRPCRequest:request error:&error];

    UIApplication *app = [UIApplication sharedApplication];
    app.networkActivityIndicatorVisible = NO;

    [self dismissWaitingAlert];

    if(error != nil || [[result body] rangeOfString:@"<name>error</name>"].location != NSNotFound)
    {
        int location_start = [[result body] rangeOfString:@"<string>"].location + 8;
        int location_end = [[result body] rangeOfString:@"</string>"].location;

        NSString *message = [[[result body] substringWithRange:NSMakeRange(location_start, location_end- location_start)] unescapedString];
        NSString* title = [UtilsHelper localizeString:@"PublishVC_ErrorRetreivingAlertTitle_key"];
        [self showAlertWithErrorMessage:message Title:title];
          return;
      }

    [self processPublishResult:result];
}

Wow! This is a really big method. And it is inside a ViewController class, so this should definitely be extracted into a service class, so we have a correct separation of concerns. But for the sake of the brevity, let's focus on how can we refactor this big method. The refactoring technique to apply here is Extract Method: you can aggregate code together and extract to a new method. So let's see what we can come up with:

We can start with grouping the code that refers to serializing a post:

- (NSString *)serializePost
{
    PostSerializer* postSerializer = [[PostSerializer alloc] init];
    NSString *post = [postSerializer serializePostWithTitle:self.txtTitle.text description:self.txtDescription.text author:self.txtUser.text game:self.game];
    return post;
}

Then we can do it for the parameters of the request:

- (NSArray *)createPostParams:(NSString *)post
{
    NSMutableDictionary *postParams = [NSMutableDictionary dictionary];
    [postParams setObject:txtTitle.text forKey:@"title"];
    [postParams setObject:post forKey:@"data"];
    [postParams setObject:txtUser.text forKey:@"username"];
    [postParams setObject:txtPassword.text forKey:@"password"];

    NSArray *args = [NSArray arrayWithObjects:[NSNumber numberWithInt:0], postParams, nil];
    return args;
}

With all this in place we are now ready to create a XMLRPCRequest:

- (XMLRPCRequest *)createXMLRPCRequestWithArgs:(NSArray*)args {

    XMLRPCRequest *request;

#ifdef DEBUG_LOG
   request = [[XMLRPCRequest alloc] initWithURL:
                              [NSURL URLWithString:@"http://localhost:8888/letsbasket/xmlrpc.php"]];
    DLog(@"Debug");
#else
    request = [[XMLRPCRequest alloc] initWithURL:[NSURL URLWithString:[UtilsHelper localizeString:@"UrlXmlRPCKey"]]];
    DLog(@"Producao");
#endif

    [request setMethod:@"letsBasket.AddPost" withParameters:args];

    return request;
}

We can also extract a method with some display updates:

- (void)updateDisplay
{
    UIApplication *app = [UIApplication sharedApplication];
    app.networkActivityIndicatorVisible = NO;

    [self dismissWaitingAlert];
}

And last but not least we can extract the preparation for displaying the error message:

- (void)showError:(NSString*)bodyResult {

    int location_start = [bodyResult rangeOfString:@"<string>"].location + 8;
    int location_end = [bodyResult rangeOfString:@"</string>"].location;

    NSString *message = [[bodyResult substringWithRange:NSMakeRange(location_start, location_end- location_start)] unescapedString];
    NSString* title = [UtilsHelper localizeString:@"PublishVC_ErrorRetreivingAlertTitle_key"];
    [self showAlertWithErrorMessage:message Title:title];
}

With all these extractions our method now looks pretty neat:

- (void) postToServer
{
    NSString *post = [self serializePost];
    NSArray *args = [self createPostParams:post];
    XMLRPCRequest *request = [self createXMLRPCRequestWithArgs: args];
    NSError *error = nil;

      XMLRPCResponse* result = [XMLRPCConnection sendSynchronousXMLRPCRequest:request error:&error];

    [self updateDisplay];

    if(error != nil || [[result body] rangeOfString:@"<name>error</name>"].location != NSNotFound)
    {
        [self showError:[result body]];
            return;
      }

    [self processPublishResult:result];
}

Hmm... we can do this even better! Let's take a look at the method createXMLRCPRequest and see if we can call the others from there. In this case, it makes sense to have all together.

- (XMLRPCRequest *)createXMLRPCRequest {

    NSString *post = [self serializePost];
    NSArray *args = [self createPostParams:post];

    XMLRPCRequest *request;

#ifdef DEBUG_LOG
   request = [[XMLRPCRequest alloc] initWithURL:
                              [NSURL URLWithString:@"http://localhost:8888/letsbasket/xmlrpc.php"]];
    DLog(@"Debug");
#else
    request = [[XMLRPCRequest alloc] initWithURL:[NSURL URLWithString:[UtilsHelper localizeString:@"UrlXmlRPCKey"]]];
    DLog(@"Producao");
#endif

    [request setMethod:@"letsBasket.AddPost" withParameters:args];

    return request;
}

And our original method now looks like this:

- (void) postToServer
{
    XMLRPCRequest *request = [self createXMLRPCRequest];
    NSError *error = nil;

      XMLRPCResponse* result = [XMLRPCConnection sendSynchronousXMLRPCRequest:request error:&error];

    [self updateDisplay];

    if(error != nil || [[result body] rangeOfString:@"<name>error</name>"].location != NSNotFound)
    {
        [self showError:[result body]];
            return;
      }

    [self processPublishResult:result];
}

Well, here you go: a method with more than 5 lines and I think that's ok. :) As we can see it's really easy to let a method grow. But it's really easy to refactor and have a cleaner code too.

Conclusion

In general, bloaters are viewed as code that, over time, "gets out of hand".

Remember, code smells sometimes can't be removed, but it's good to know that they are there and you know why they are there.

This post was cross-posted to my personal blog.


In the last post, Code Smells - Part I, I talked about the bloaters: they are code smells that can be identified as Long Methods, Large Classes, Primitive Obsessions, Long Parameter List and Data Clumps. In this one, I would like to dig into the Object-Orientation Abusers and the Change Preventers.

Object-Orientation Abusers

This type of code smell usually happens when object-oriented principles are incomplete or incorrectly applied.

Switch Statements

This case is simple to identify: we have a switch case. But you should consider it a smell too if you find a sequence of ifs. (That's a switch case in disguise.)

Why are switch statements bad? Because when a new condition is added, you have to find every occurrence of that switch case.

So while talking to David, he asked me: and what happens if I encapsulate the switch into a method, is it acceptable then? That's really a good question... If your switch case is only used to "take care" of one behaviour and that's it, then it might be ok. Remember identifying a code smell doesn't mean that you have to get always rid of it: it's a trade off. If you find your switch statement replicated and each replication has different behaviour, then you cannot simply isolate the switch statement in a method. You need to find a proper "home" for it to be in. As a rule of thumb, you should think of polymorphism when you find yourself in this situation. There are two refactoring techniques that we can apply here:

  • Replace Type Code with Subclasses This technique consists of creating subclasses for each switch case and applying the respective behaviour to these subclasses.
  • Replace Type Code With Strategy Similar to the above one, in this case, you should make use of one of the patterns: State or Strategy.

So when to use one or the other? If the Type Code does not change the behaviour of a class you can use the Subclasses technique. Separating each behaviour into its appropriate subclass will enforce the Single Responsibility Principle and make the code more readable in general. If you need to add another case, you just add a new class to your code without having to modify any other code. So you apply the Open/Closed Principle.

You should use the Strategy approach when the Type Code affects the behaviour of your classes. If you're changing the state of the class, fields and many other actions then you should use the State Pattern. If it only affects how you select a behaviour of the class then the Strategy Pattern is a better choice.

Hmm... It's a little confusing, no? So let's try with an example.

You have an enumeration EmployeeType:

public enum EmployeeType 
{

    Worker,

    Supervisor,

    Manager

}

And a class Employee:

public class Employee

{

    private float salary;

    private float bonusPercentage;

    private EmployeeType employeeType;



    public Employee(float salary, float bonusPercentage, EmployeeType employeeType)

    {

        this.salary = salary;

        this.bonusPercentage = bonusPercentage;

        this.employeeType = employeeType;

    }



    public float CalculateSalary() 

    {

        switch (employeeType) 

        {

            case EmployeeType.Worker:

                return salary; 

            case EmployeeType.Supervisor:

                return salary + (bonusPercentage * 0.5F);

            case EmployeeType.Manager:

                return salary + (bonusPercentage * 0.7F);

        }

        return 0.0F;

    }
}

It all looks ok. But what happen if you need to calculate the year bonus? You will add another method like this:

public float CalculateYearBonus() 

{

    switch (employeeType) 

    {

        case EmployeeType.Worker:

            return 0; 

        case EmployeeType.Supervisor:

            return salary + salary * 0.7F;

        case EmployeeType.Manager:

            return salary + salary * 1.0F;  

    }

    return 0.0F;
}

See the repetition of the switch? So let's try first the subclass approach: Here is the superclass:

abstract public class Employee

{ 



protected float salary;

    protected float bonusPercentage;



    public Employee(float salary, float bonusPercentage)

    {

        this.salary = salary;

        this.bonusPercentage = bonusPercentage;

    }



    abstract public float CalculateSalary();



virtual public float CalculateYearBonus() 

    {

        return 0.0F;

    }

}

And here we have the subclasses:

public class Worker: Employee

{


    public Worker(float salary, float bonusPercentage)

        : base(salary, bonusPercentage)

    {}



     override public float CalculateSalary() 

     {

        return salary; 

     }

}

public class Supervisor : Employee

{


    public Supervisor(float salary, float bonusPercentage)

            : base(salary, bonusPercentage)

    {}



    override public float CalculateSalary() 

    {

        return salary + (bonusPercentage * 0.5F);

    }



    public override float CalculateYearBonus()

    {

        return salary + salary * 0.7F;

    }

}

With the Strategy approach we would create an interface for calculating the remuneration:

public interface IRemunerationCalculator 

{

    float CalculateSalary(float salary);

    float CalculateYearBonus(float salary);

}

With the interface in place, we can now pass to the employee any class that conforms to that protocol and calculate the correct salary/bonus.

public class Employee
{

    private float salary;

    private IRemunerationCalculator remunerationCalculator;



    public Employee(float salary, IRemunerationCalculator remunerationCalculator)

    {
        this.salary = salary;

        this.remunerationCalculator = remunerationCalculator;

    }



    public float CalculateSalary()

    {

        return remunerationCalculator.CalculateSalary(salary);

    }



    public float CalculateYearBonus() 

    {

        return remunerationCalculator.CalculateYearBonus(salary);

    }
}

Temporary Field

This case occurs when we are calculating a big algorithm that needs several input variables. Creating these fields in the class has no value most of the time because they are just used for this specific calculation. And this can be dangerous too because you have to be sure you reinitialize them before you start the next computation.

Here the best refactoring technique is to use Replace Method with Method Object, which will extract the method into a separate class. Then you can split the method into several methods within the same class.

Refused Bequest

This code smell is a little tricky to detect because this happens when a subclass doesn't use all the behaviours of its parent class. So it's as if the subclass "refuses" some behaviours ("bequest") of its parent class.

In this case, if it makes no sense to continue to use inheritance, the best refactoring technique is to change to Delegation: we can get rid of the inheritance by creating a field of the parent's classes type in our subclass. This way every time you need the methods from the parent class you just delegate them to this new object.

When the inheritance is the correct thing to do, then move all unnecessary fields and methods from the subclass. Extract all methods and fields from the subclass and parent class and put them in a new class. Make this new class the SuperClass, from whom the subclass and parent class should inherit. This technique is called Extract Superclass.

Alternative Classes with Different Interfaces

Hmm, this case makes me think of "lack of communication" between members of the same team because this happens when we have two classes that do the same thing but have different names for their methods. Start by Renaming Methods or Moving Method, so you can have both classes implementing the same interface. In some cases, only part of the behaviour is duplicated in both classes. If so, try Extract Superclass and make the original classes the subclasses.

Change Preventers

Oh boy! This kind of code smells are the ones you really want to avoid. These are the ones that when you make a change in one place, you have to go basically throughout your code-base making changes in other places too. So it's a nightmare that all of us want to avoid!

Divergent Change

This is the case when you find yourself changing the same class for several different reasons. This means that you are violating the Single Responsibility Principle (which has to do with separation of concerns).

The refactoring technique applied here is Extract Class since you want to extract the different behaviours into different classes.

Shotgun Surgery

This means that when you make a small change in a class, you have to go and change several classes at the same time.

Even though it seems the same as the Divergent Change smell, in reality, they are opposite of each other: Divergent Change is when many changes are made to a single class. Shotgun Surgery refers to when a single change is made to multiple classes simultaneously.

Here the refactoring technique to apply is Move Method and/or Move Field. This will permit you to move the duplicated methods or fields to a common class. If that class doesn't exist, create a new one. In the case where the original class stays almost empty, maybe you should think if this class is redundant, and if so, get rid of it by using Inline Class: move the remaining methods/fields to one of the new classes created. This all depends on whether the original class still has any responsibilities.

Parallel Inheritance Hierarchies

This case is when you find yourself creating a new subclass for class B because you add a subclass to class A.

Here you can: first, make one of the hierarchy refer to instances of another hierarchy. After this first step you can then use Move Method and Move Field to remove the hierarchy in the referred class. You can apply here the Visitor pattern too.

Conclusion

In the case of Object-Orientation Abusers and Change Preventers, I think that they are simpler to avoid if you know how to apply a good design to your code. And that comes with a lot of practice.

Today I've talked about a few refactoring techniques, but there are a lot more. You can find a good reference to all of then in Refactoring.com.

And as I said in the first part of this series, code smells can't always be removed. Study each case and decide: remember there is always a trade-off.

This post was cross-posted to my personal blog, and it was also published on the Codurance blog."

Moving towards a private or Hybrid cloud infrastructure model? Get started with our OpenStack Deployment Models guide to learn the proper deployment model for your organization.

Topics:
code smell ,parameter ,java ,clean code

Published at DZone with permission of Ana Nogal, DZone MVB. See the original article here.

Opinions expressed by DZone contributors are their own.

THE DZONE NEWSLETTER

Dev Resources & Solutions Straight to Your Inbox

Thanks for subscribing!

Awesome! Check your inbox to verify your email so you can start receiving the latest in tech news and resources.

X

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}