Software Engineering - Refactoring and Code Smell

Refactoring

Definition: Improving design without changing functionality

• Allows incremental design evolution

• automated test suiter help ensure that refactoring preserves the functionality

  • We use Unit Test to check that the Refactoring is successful!

• Triggering for refactoring

  • adding new feature - need to change design to implement the feature
  • “Code smells” : indication of potential design problem

Sample code smells

Definition: Indicators of Potential Problems

A code smell, if you’ll recall, is a superficial characteristic of code that is often indicative of deeper problems. It’s similar in concept to the term “red flag” for interpersonal relationships.

• Large method (function)

  • What is large => depends on program language
  • Benefits of breaking up large function into smaller pieces
    • easier to understand (modularization, naming functions to improve readability)
    • may be able to reuse code
    • easier to test => can write unit tests for each part

• Large class (“God class”)

  • Large = several dozens and up
  • Class that assures too many responsibilities
  • Refactoring: break up into smaller classes => e.g. introducing a design pattern

• Feature Envy/ Long parameter list (too many arguments!)

  • the method has too many responsibilities
  • for library code, it is okay. but for application code, it is not!

• Data clamps

  • set of variables always passed together

  • print Rectangle(ul_x int, ul_y int, br_x int, br_y int, color Color)

    • Refactoring: group variables into meaning uses
      • print Rectangle(ulc Point, brc Point, color Color)

• Excessive method chain:

  • e.g. company.getDivisions().getTeams().getEmployee().getSalary()

• Duplicate Code

  • Potential source of bugs => if chance to one clone requires updating other clones

  • Increased maintence because more code

  • Key question: what is the intent for duplications?

    • are the cones intended to evolve together or to diverge
    • => Achieve no coupling!

• Comment

  • a clarification comment is a code smell. It tells you that your code is too complex.
  • good code is self-documenting

• Switch Statement

  • Difficult to maintain
  • Switching on type is a bad OOP style
  • Adding new type involve updating the whole thing!

• Magic number

  • unexplained constants
    • they don’t give any indication as to why they’re present

• Primitive obession

  • primitive data types are used excessively
  • For value types (number + structure + logic)

• Speculative generality

  • too flexible, assumed too much
  • e.g. using Message Strategy to print out “Hello World”

• Shotgun surgery

  • Modifing one requriement need to changing too much places in the code
  • Hard to refractor

Some solutions to tackle the code smell

• Large method

  • pulled out (extract) methods + Refactoring Switch

• Comments

  • pulled out (extract) methods

• Switch Statement

  • Strategy design with Subclass and Polymorphism

• Primitive Obsession

  • Introduce classes
  • E.g. We should use Money class for currency
    • Why? Because money can be encapsulated with methods e.g. currency conversation, rounding etc.

Large method Example:

Switch Statement Example:

Practice Questions

An Example: Video Store Inventory

Consider a program that calculates and prints a statement of a customer’s charges at a video store:

1. Movies are rented by customers for a given number of days.
2. Three kinds of movies: regular, children’s and new releases.
3. Customers accumulate frequent renter “points”.

Class diagram:

• Movie can have 0…* rental, but a rental must have 1 movie
• Rental can only have 1 customer, but a customer can have 0…* rental

Java implementation:

public class Movie {
    public static final int REGULAR = 0;
    public static final int NEW_RELEASE = 1;
    public static final int CHILDRENS = 2;

    private String _title;
    private int _priceCode;
    public Movie(String title, int priceCode) {
        _title = title;
        _priceCode = priceCode;
    }

    public int getPriceCode() {
        return _priceCode;
    }
    public void setPriceCode(int arg) {
        _priceCode = arg;
    }
    public String getTitle() {
        return _title;
    }
}

public class Rental {
    private Movie _movie;
    private int _daysRented;

    public Rental(Movie movie, int daysRented) {
        _movie = movie;
        _daysRented = daysRented;
    }

    public int getDaysRented() {
        return _daysRented;
    }
    public Movie getMovie() {
        return _movie;
    }
}

class Customer {
    private String _name;
    private java.util.Vector _rentals = new java.util.Vector();

    public Customer(String name) {
        _name = name;
    }

    public void addRental(Rental arg) {
        _rentals.addElement(arg);
    }
    public String getName() {
        return _name;
    }

    public String statement() {
        double totalAmount = 0;
        java.util.Enumeration rentals = _rentals.elements();
        String result = "Rental Record for " + getName() + "\n";
        while (rentals.hasMoreElements() == true) {
            double thisAmount = 0;
            Rental each = (Rental) rentals.nextElement();
            // Determine amounts for each line of movies 
            switch (each.getMovie().getPriceCode()) {
                case Movie.REGULAR:
                    thisAmount += 2;
                    if (each.getDaysRented() > 2) {
                        thisAmount += (each.getDaysRented() - 2) * 1.5;
                    }
                    break;
                case Movie.NEW_RELEASE:
                    thisAmount += each.getDaysRented() * 3;
                    break;
                case Movie.CHILDRENS:
                    thisAmount += 1.5;
                    if (each.getDaysRented() > 2) {
                        thisAmount += (each.getDaysRented() - 2) * 1.5;
                    }
                    break;
            }
            // Show figures for this rental
            result += "\t" + each.getMovie().getTitle() + "\t" +
                String.valueOf(thisAmount) + "\n";
            totalAmount += thisAmount;
        }
        // Add footer line
        result += "Amount owed is " + String.valueOf(totalAmount) + "\n";
        return result;
    }
}

Based on the code above, please answer the following questions.

Question 1 Identify at least four code smells in this implementation. What are they? Where do they occur in the code (name the relevant program element(s))?

Smell: Large method

Location: Customer.statment()

(long because showing too much detail: calculation and printing, and because of switch statement)

Smell: Switch Statement

Location: Customer.statment()

(difficult to maintain)

Smell: Magic number

Location: Customer.statment()

Smell: Comments

Location: Customer.statment()

Smell: Large Method

Location: Customer.statement()

Smell: Code duplication

Location: Customer.statement()

Smell: Primitive Obession

Location: static fields: REGULAR, NEW_RELEASE, CHILDRENS
Also: representing money as double rather than a Money class

Question 2 Please draw a class diagram of a refactored design that addresses these smells. Indicate the content of the key methods in your design. What design patterns or refactoring have you applied?

My answer:

• Recall the original one:
• 
• Then apply strategy method
  • Movie is the Context
  • MovieType is the Strategy
    • StrategyA : Regular, StrategyB: NewRelase, StrategyC: Children

Model answer:

The class diagram and below are the result of refactoring to addresses the smells listed above. “Primitive Obsession” and “Switch statement” were addressed by refactoring to the Strategy pattern (MovieType). “Code duplication” was addressed by applying Template Method in MovieType. “Long method” was addressed by extracting the steps of statement printing as separate, private methods. “Comments” was addressed by defining methods explaining the constants.