Code smell

Bloaters

Bloaters are code, methods and classes that have increased to such gargantuan proportions that they’re hard to work with. Usually these smells don’t crop up right away, rather they accumulate over time as the program evolves (and especially when nobody makes an effort to eradicate them).

Long method

A method contains too many lines of code. Generally, any method longer than ten lines should make you start asking questions.

class OrderProcessor {
    public void processOrder(Order order) {
        // Validate order
        if (order.getItems().isEmpty()) {
            throw new IllegalArgumentException("Order is empty");
        }
        if (order.getCustomer() == null) {
            throw new IllegalArgumentException("Customer is missing");
        }
        
        // Calculate totals
        double subtotal = 0;
        for (Item item : order.getItems()) {
            subtotal += item.getPrice() * item.getQuantity();
        }
        double tax = subtotal * 0.1;
        double total = subtotal + tax;
        
        // Apply discount
        if (order.getCustomer().isVip()) {
            total = total * 0.9;
        }
        
        // Update inventory
        for (Item item : order.getItems()) {
            Inventory.decrease(item.getId(), item.getQuantity());
        }
        
        // Save order
        order.setTotal(total);
        Database.save(order);
    }
}

Large class

A class contains many fields/methods/lines of code.[

class UserManager {
    private String username;
    private String password;
    private String email;
    private String address;
    private String phoneNumber;
    private List<Order> orders;
    private PaymentDetails paymentDetails;
    private ShippingPreferences shippingPreferences;
    
    public void createUser() { /* ... */ }
    public void updateProfile() { /* ... */ }
    public void changePassword() { /* ... */ }
    public void processOrder() { /* ... */ }
    public void calculateShipping() { /* ... */ }
    public void handlePayment() { /* ... */ }
    // Many more methods...
}

Primitive Obsession

• Use of primitives instead of small objects for simple tasks (such as currency, ranges, special strings for phone numbers, etc.)
• Use of constants for coding information (such as a constant USER_ADMIN_ROLE = 1 for referring to users with administrator rights.)
• Use of string constants as field names for use in data arrays.

class User {
    private String phoneNumber;  // Format: "XXX-XXX-XXXX"
    private int userType;        // 1 = admin, 2 = regular, 3 = guest
    private double money;        // USD
    
    public boolean isAdmin() {
        return userType == 1;
    }
}

Long Parameter List

More than three or four parameters for a method.

class Report {
    public void generateReport(String title, String date, String author, 
                             String content, String format, String fontSize,
                             String color) {
        // Generate report with all parameters
    }
}

Data Clumps

Sometimes different parts of the code contain identical groups of variables (such as parameters for connecting to a database). These clumps should be turned into their own classes.

class OrderProcessor {
    public void connect(String url, String username, String password, int port) {
        // Connect to database
    }
}

class ReportGenerator {
    public void connect(String url, String username, String password, int port) {
        // Connect to database
    }
}

Object-Orientation Abuser

Switch Statements

You have a complex switch operator or sequence of if statements.

class PaymentProcessor {
    double processPayment(String paymentType, double amount) {
        double fee;
        switch (paymentType) {
            case "CREDIT_CARD":
                fee = amount * 0.03;
                break;
            case "DEBIT_CARD":
                fee = amount * 0.02;
                break;
            case "PAYPAL":
                fee = amount * 0.04;
                break;
            default:
                fee = 0;
        }
        return amount + fee;
    }
}

Temporary Field

Temporary fields get their values (and thus are needed by objects) only under certain circumstances. Outside of these circumstances, they’re empty.

class Order {
    private double subtotal;
    private double tax;  // Only used during calculation
    private double discount;  // Only used during calculation
    
    public double calculateTotal() {
        tax = subtotal * 0.1;
        discount = subtotal > 100 ? subtotal * 0.05 : 0;
        return subtotal + tax - discount;
    }
}

Refused Bequest

If a subclass uses only some of the methods and properties inherited from its parents, the hierarchy is off-kilter. The unneeded methods may simply go unused or be redefined and give off exceptions.

class Bird {
    void fly() { /* implementation */ }
    void makeSound() { /* implementation */ }
    void layEggs() { /* implementation */ }
}

class Penguin extends Bird {
    @Override
    void fly() {
        throw new UnsupportedOperationException("Penguins can't fly!");
    }
}

Alternative Classes with Different Interfaces

Two classes perform identical functions but have different method names.

class UserFileHandler {
    void saveUser(User user) { /* saves to file */ }
    User loadUser(int id) { /* loads from file */ }
}

class UserDbManager {
    void writeUserToDatabase(User user) { /* saves to DB */ }
    User getUserFromDatabase(int id) { /* loads from DB */ }
}

Change Preventers

These smells mean that if you need to change something in one place in your code, you have to make many changes in other places too. Program development becomes much more complicated and expensive as a result.

Divergent Change

You find yourself having to change many unrelated methods when you make changes to a class. For example, when adding a new product type you have to change the methods for finding, displaying, and ordering products.

class Product {
    private String name;
    private double price;

    // Methods change for different reasons
    public String getDisplayInfo() {
        if (isBook()) {
            return name + " - Author: " + getAuthor();
        } else if (isElectronics()) {
            return name + " - Warranty: " + getWarranty();
        }
        return name;
    }

    public double calculateShipping() {
        if (isBook()) {
            return 2.99;
        } else if (isElectronics()) {
            return 9.99;
        }
        return 4.99;
    }

    public String getInventoryStatus() {
        if (isBook()) {
            return "Books section, aisle " + getBookAisle();
        } else if (isElectronics()) {
            return "Electronics section, shelf " + getElectronicsShelf();
        }
        return "General section";
    }
}

Shotgun Surgery

Making any modifications requires that you make many small changes to many different classes.

class Customer {
    private String email;
    
    public void sendEmail(String message) {
        System.out.println("Sending email to: " + email);
    }
}

class Order {
    private String customerEmail;
    
    public void notifyCustomer() {
        System.out.println("Sending email to: " + customerEmail);
    }
}

class Support {
    private String userEmail;
    
    public void sendTicketUpdate() {
        System.out.println("Sending email to: " + userEmail);
    }
}

Parallel Inheritance Hierarchies

Whenever you create a subclass for a class, you find yourself needing to create a subclass for another class.

class Animal {
    protected String name;
}

class Dog extends Animal {
    public void bark() {}
}

class Cat extends Animal {
    public void meow() {}
}

class AnimalFeeder {
    protected String foodType;
}

class DogFeeder extends AnimalFeeder {
    public void feedDog() {}
}

class CatFeeder extends AnimalFeeder {
    public void feedCat() {}
}

Dispensables

A dispensable is something pointless and unneeded whose absence would make the code cleaner, more efficient and easier to understand.

Comments

A method is filled with explanatory comments.

class UserValidator {
    public boolean validateUser(User user) {
        // Check if the user is null
        if (user == null) {
            return false;
        }
        
        // Check if the username is empty
        if (user.getUsername().isEmpty()) {
            return false;
        }
        
        // Check if the email is valid
        if (!user.getEmail().contains("@")) {
            return false;
        }
        
        // If all checks pass, return true
        return true;
    }
}

Duplicate Code

Two code fragments look almost identical.

class PaymentProcessor {
    public double calculateRegularPayment(Order order) {
        double total = 0;
        for (Item item : order.getItems()) {
            double price = item.getPrice();
            double tax = price * 0.2;
            double shipping = 5.0;
            total += price + tax + shipping;
        }
        return total;
    }
    
    public double calculatePrimePayment(Order order) {
        double total = 0;
        for (Item item : order.getItems()) {
            double price = item.getPrice();
            double tax = price * 0.2;
            double shipping = 0.0;  // Only difference is free shipping
            total += price + tax + shipping;
        }
        return total;
    }
}

Lazy Class

Understanding and maintaining classes always costs time and money. So if a class doesn't do enough to earn your attention, it should be deleted.

class Address {
    private String street;
    private String city;
}

class AddressFormatter {
    public String format(Address address) {
        return address.getStreet() + ", " + address.getCity();
    }
}

Data Class

A data class refers to a class that contains only fields and crude methods for accessing them.

class Customer {
    private String name;
    private String email;
    private int age;

    public String getName() { return name; }
    public void setName(String name) { this.name = name; }
    public String getEmail() { return email; }
    public void setEmail(String email) { this.email = email; }
    public int getAge() { return age; }
    public void setAge(int age) { this.age = age; }
}

Dead Code

A variable, parameter, field, method or class is no longer used.

class OrderProcessor {
    private boolean isLegacySystem = false;  // Never used
    
    public void processOrder(Order order) {
        validateOrder(order);
        // Old payment method, not used anymore
        if (isLegacySystem) {
            processLegacyPayment(order);
        }
        saveOrder(order);
    }
    
    private void processLegacyPayment(Order order) {
        // Legacy payment logic
    }
}

Speculative Generality

There's an unused class, method, field or parameter.

interface Animal {
    void eat();
    void sleep();
    void fly();  // What if some animals need to fly?
    void swim();  // What if some animals need to swim?
}

class Cat implements Animal {
    public void eat() { /* implementation */ }
    public void sleep() { /* implementation */ }
    public void fly() { /* empty - cats don't fly */ }
    public void swim() { /* empty - most cats don't swim */ }
}

Couplers

All the smells in this group contribute to excessive coupling between classes or show what happens if coupling is replaced by excessive delegation.

Feature Envy

A method that's more interested in another class than the one it's in.

class Order {
    private Customer customer;
    
    public double calculateTotal() {
        double basePrice = customer.getBasePrice();
        double discount = customer.getDiscount();
        double tax = customer.getTaxRate();
        
        return basePrice * (1 - discount) * (1 + tax);
    }
}

Inappropriate Intimacy

Classes that are too tightly coupled, accessing each other's private members.

class Student {
    private List<Course> courses;
    
    public void enrollInCourse(Course course) {
        courses.add(course);
        course.studentList.add(this); // Directly accessing private field
        course.numberOfStudents++; // Directly accessing private field
    }
}

class Course {
    List<Student> studentList;
    private int numberOfStudents;
}

Message Chains

Long chains of method calls that create tight coupling between classes.

class Client {
    public void printUserAddress() {
        String zip = getCompany()
            .getDepartment()
            .getManager()
            .getAddress()
            .getZipCode();
        System.out.println(zip);
    }
}

Middle Man

A class that delegates all its work to another class.

class PersonProxy {
    private Person person;
    
    public String getName() {
        return person.getName();
    }
    
    public void setName(String name) {
        person.setName(name);
    }
    
    public String getAddress() {
        return person.getAddress();
    }
    
    public void setAddress(String address) {
        person.setAddress(address);
    }
}