Code Smells

Learning Objectives

Recognise the main OO code smells, their symptoms and impact.

Choose a proportionate refactoring for each.

Prerequisites: Modules 19–21.

Intuition and Definition

A code smell is a surface symptom that usually indicates a deeper design problem. Smells are not bugs; they make bugs more likely and change more expensive.

One class knows/does everything (thousands of lines) Extract class by responsibility

Many levels of nesting, comments separating "steps" A method uses another class's data more than its own

The same 3–4 parameters always travel together Introduce parameter object / value object Strings/ints for domain concepts Value objects (Module 18)

One change requires edits in many classes Move/inline to gather the knowledge

One class changes for many unrelated reasons Extract class (SRP) The same rule in several places Extract and centralize the rule

Classes poke into each other's private parts Move members, introduce interfaces

Subclass ignores or throws for inherited members Replace inheritance with composition

Hooks and abstractions for imagined futures Inline, remove (YAGNI) Modules depend on each other Dependency inversion, extract module render(true, false, true)

Separate methods, enums, parameter objects Flatten, compose Entities with only getters/setters Move behaviour into the domain

Interfaces with one implementation everywhere Inline until a second implementation is real

Why It Matters

Smells are the vocabulary of code review. Naming the smell ("this is feature envy") points directly at the fix.

Syntax and Language Notes

Static analysers detect some smells automatically: clang-tidy, SonarQube, PMD, ReSharper/Rider inspections, pylint, ESLint complexity rules.

Complete Example

A small analyser that flags two measurable smells — long methods and boolean-flag parameters — in Python source using the standard ast module:

import ast, textwrap

source = textwrap.dedent('''
    class ReportService:
        def render(self, data, as_pdf, include_logo, landscape):
            if as_pdf:
                pass
            return data

        def total(self, lines):
            t = 0
            for l in lines:
                if l.qty > 0:
                    if l.price > 0:
                        if not l.cancelled:
                            t += l.qty * l.price
            return t
''')

class SmellFinder(ast.NodeVisitor):
    def visit_FunctionDef(self, fn):
        flags = [a.arg for a in fn.args.args if a.arg.startswith(("as_", "include_", "is_")) or a.arg in ("landscape",)]
        if len(flags) >= 2:
            print(f"{fn.name}: boolean-flag parameters {flags} -> split method or use an options object")
        depth = self.max_depth(fn, 0)
        if depth >= 4:
            print(f"{fn.name}: nesting depth {depth} -> extract method / guard clauses")
        self.generic_visit(fn)

    def max_depth(self, node, d):
        best = d
        for child in ast.iter_child_nodes(node):
            nd = d + 1 if isinstance(child, (ast.If, ast.For, ast.While, ast.With, ast.Try)) else d
            best
…

render: boolean-flag parameters ['as_pdf', 'include_logo', 'landscape'] -> split method or use an options object
total: nesting depth 4 -> extract method / guard clauses

Step-by-Step Walkthrough

The visitor walks every function definition in the parsed source.

render takes three flag-like booleans — callers will write render(d, True, False, True) and nobody can read it.

total nests four levels deep; guard clauses (if l.qty <= 0: continue) or an extracted predicate would flatten it.

Common Mistakes

Treating smells as rules — a 60-line method that reads top-to-bottom may be fine.

Refactoring smells without tests — behaviour changes slip in.

Fixing every smell at once in an unrelated change — keep refactorings separate and small.

Trade-offs and Alternatives

Refactor proportionately: fix the smell in code you're changing anyway (boy-scout rule); schedule larger restructurings when the smell is actively slowing work.

Real-World Use

Code review comments, static-analysis quality gates in CI, technical-debt registers that link smells to the features they slow down.

Practice

Q1. (Easy) What smell is void process(bool fast, bool retry, bool verbose)?

Answer: Boolean-flag parameters.

Q2. (Medium) Order::price() reads customer.tier, customer.region and customer.discounts but no order fields except the total. Which smell?

Answer: Feature envy — the logic probably belongs in Customer (or a pricing policy taking the customer).

Q3. (Advanced) What is the difference between shotgun surgery and divergent change?

Answer: Shotgun surgery: one change requires edits in many classes (knowledge scattered). Divergent change: one class is edited for many different reasons (responsibilities lumped). They are mirror images: the fix for shotgun surgery is to gather; for divergent change it is to split.

Interview Question

Q1. "You inherit a 5,000-line god class. How do you start?"

Answer: Don't rewrite it. Add characterization tests around current behaviour (especially public entry points), map its responsibilities (group methods and fields by what they touch), then extract one cohesive responsibility at a time into new classes that the god class delegates to, keeping behaviour identical and tests green; repeat, prioritising the parts that change most often. Use IDE refactorings to reduce risk and keep each step small and reviewable.

Quick Quiz

Q1. Is a code smell a bug?

Answer: No — it's a symptom that makes bugs and costly changes more likely.

Q2. Fix for data clumps?

Answer: Introduce a parameter object or value object.

Q3. Fix for refused bequest?

Answer: Replace inheritance with composition (or redesign the hierarchy).

Summary

Smells name design problems; each suggests a refactoring.

Refactor proportionately and with tests.

Related: Modules 53, 54.