Detailed design and code quality

Architecture decides the big structure; detailed design decides how each module, class and function is organized inside. Most of a developer's working life is spent at this level, reading and changing code that someone else wrote, often months or years ago. Code that is easy to read and change keeps a product moving; code that is tangled slows every change and hides defects. This lesson covers the core principles of good detailed design, coupling, cohesion and information hiding; how to recognize code smells and remove them by refactoring safely under tests; how to measure complexity; how linters, formatters and type checkers help; and how to think about technical debt. CMPUT-274 lessons 5 and 6 covered SOLID principles and design patterns; this lesson builds on them with a focus on maintaining and improving existing code.

Coupling and cohesion

Two concepts from structured design in the 1970s, by Larry Constantine and Edward Yourdon, remain the most useful ways to judge a design. Coupling is the degree to which one module depends on others. Cohesion is the degree to which the elements inside one module belong together. The goal is low coupling and high cohesion: modules that each do one well-defined job and depend on each other through small, stable interfaces. Low coupling means a change in one module rarely forces changes in others; high cohesion means that when a change is needed, it is in one place.

Cohesion has a similar ladder, from coincidental (unrelated functions in a utils module), through logical (functions grouped because they are the same kind of thing, such as all input handling), temporal (things done at the same time, such as everything at start-up), to functional cohesion, where everything in the module contributes to one well-defined task. A useful smell test: if describing what a module does requires the word 'and', it probably has low cohesion.

David Parnas's 1972 paper on information hiding gave the reason these principles work. He argued that a system should be decomposed so that each module hides a design decision likely to change, such as a data format, an algorithm or the choice of an external service, behind an interface. When that decision changes, only the module that hides it changes. A notifier interface hides whether reminders go by email, text message or app notification; a repository hides whether shifts are stored in PostgreSQL or SQLite. Python has no enforced private fields, but its conventions (a leading underscore, properties, modules with a small public API listed in __all__) serve the same purpose.

Code smells

Kent Beck and Martin Fowler coined the term code smell for a surface sign that usually indicates a deeper design problem. A smell is not a bug, and not every smell must be removed, but each is worth a second look. Fowler's book Refactoring, first published in 1999 and revised in 2018, catalogs smells and the refactorings that remove them.

Refactoring safely

Refactoring is changing the internal structure of code without changing its observable behavior, in small steps, each of which keeps the program working. The safety net is a suite of tests: run them before the change, make one small refactoring, run them again, and commit. If a test fails, the last step was small enough to undo and redo carefully. Refactoring without tests is just editing and hoping. When legacy code has no tests, the first step is to write characterization tests, which record what the code currently does, right or wrong, so that changes in behavior are noticed; Michael Feathers's Working Effectively with Legacy Code is the standard guide.

Here is a function with several smells: a flag argument, primitive lists indexed by number, nested conditions, a hard-coded month, string concatenation, and an error that prints instead of failing. First a characterization test captures its current behavior.

cat test_report.py
pytest -q test_report.py

$ cat test_report.py
from report import report
ROWS = [["Ana", "Pantry", "2026-10-03", "3", "attended"],
        ["Ben", "Pantry", "2026-10-03", "3", "no_show"],
        ["Chloe", "Delivery", "2026-10-04", "4.5", "attended"],
        ["Ana", "Delivery", "2026-11-01", "2", "attended"]]
def test_month_report():
    assert report(ROWS, "month") == ["Ana Pantry 3", "Chloe Delivery 4.5", "TOTAL 7.5"]
def test_program_report():
    assert report(ROWS, "program") == ["Pantry 3.0", "Delivery 6.5"]
$ pytest -q test_report.py
..                                                                       [100%]
2 passed in 0.01s

def report(rows, kind):
    # rows: list of [name, program, date, hours, status]
    out = []
    total = 0
    if kind == "month":
        for r in rows:
            if r[4] == "attended":
                if r[2][:7] == "2026-10":
                    total = total + float(r[3])
                    out.append(r[0] + " " + r[1] + " " + str(r[3]))
        out.append("TOTAL " + str(total))
    elif kind == "program":
        progs = {}
        for r in rows:
            if r[4] == "attended":
                if r[1] in progs:
                    progs[r[1]] = progs[r[1]] + float(r[3])
                else:
                    progs[r[1]] = float(r[3])
        for p in progs:
            out.append(p + " " + str(progs[p]))
    else:
        print("bad kind")
    return out

The refactoring proceeds in small steps: introduce a dataclass for a row (replace primitive with object), extract the repeated attended filter, split the flag argument into two functions, make the month a parameter, and use f-strings and a defaultdict. After each step the tests run; at the end the tests are updated to call the new functions, which is the one deliberate change to the interface. The result is shorter and each function has one job.

from collections import defaultdict
from dataclasses import dataclass


@dataclass(frozen=True)
class Attendance:
    name: str
    program: str
    date: str          # ISO date, for example "2026-10-03"
    hours: float
    status: str


def attended(rows: list[Attendance]) -> list[Attendance]:
    return [r for r in rows if r.status == "attended"]


def monthly_lines(rows: list[Attendance], month: str) -> list[str]:
    selected = [r for r in attended(rows) if r.date.startswith(month)]
    lines = [f"{r.name} {r.program} {r.hours:g}" for r in selected]
    lines.append(f"TOTAL {sum(r.hours for r in selected):g}")
    return lines


def hours_by_program(rows: list[Attendance]) -> dict[str, float]:
    totals: dict[str, float] = defaultdict(float)
    for r in attended(rows):
        totals[r.program] += r.hours
    return dict(totals)

pytest -q test_report.py

$ pytest -q test_report.py
...                                                                      [100%]
3 passed in 0.01s

Look at the third test. Because the month is now a parameter, testing November costs one line; with the hard-coded month, it would have required editing the function. Refactoring that improves testability is often the best evidence that a design improved.

Measuring complexity

Thomas McCabe's cyclomatic complexity, from 1976, counts the number of independent paths through a function: one plus the number of decision points (each if, elif, for, while, and, or, except and similar). A function with complexity 1 has one path; a function with complexity 10 needs at least 10 test cases to exercise every branch, and is harder to understand. A common guideline is to keep functions under 10. Tools compute it automatically. Here flake8 with its McCabe plugin and the Ruff linter report the two versions of the report function.

flake8 --select=C901 --max-complexity=3 report_before.py report_after.py
ruff check --select C901 --config "lint.mccabe.max-complexity=3" --output-format concise report_before.py report_after.py

$ flake8 --select=C901 --max-complexity=3 report_before.py report_after.py
report_before.py:1:1: C901 'report' is too complex (10)
$ ruff check --select C901 --config "lint.mccabe.max-complexity=3" --output-format concise report_before.py report_after.py
report_before.py:1:5: C901 `report` is too complex (10 > 3)
Found 1 error.

The original function has complexity 10 and is flagged; none of the refactored functions exceeds 3, so neither tool reports them. Complexity is not the whole story: a long sequence of simple statements can still be hard to read, and a dictionary dispatch can hide complexity from the metric. Cognitive complexity, a measure introduced by SonarSource in 2017, weights nested structures more heavily, matching how hard code feels to read. Metrics are useful for finding hotspots, especially when combined with change frequency: a complex file that changes every week is a far better refactoring target than a complex file nobody has touched in years.

Linters, formatters and type checkers

Automated tools catch whole categories of problems before review. A formatter such as Black or Ruff's formatter rewrites code into one consistent style, so reviews never argue about spacing. A linter such as Ruff, flake8 or Pylint flags likely bugs and style violations: unused imports and variables, undefined names, mutable default arguments, comparisons with None using ==, and hundreds more. A static type checker such as mypy or Pyright checks type hints, catching errors such as passing a string where a number is expected, as CMPUT-274 lesson 7 showed. Running all three automatically in the editor and in continuous integration (lesson 11) moves a large share of review comments to the machine.

cat notify.py
ruff check --select F,E711,B006 --output-format concise notify.py
mypy --strict --no-error-summary --no-color-output notify.py

$ cat notify.py
import os
import json
def reminders(shifts, sent=[]):
    for s in shifts:
        if s["volunteer"] == None:
            continue
        sent.append(s["volunteer"])
    return sent
def hours(shift: dict) -> int:
    return shift["end"] - shift["start"] + "h"
$ ruff check --select F,E711,B006 --output-format concise notify.py
notify.py:1:8: F401 [*] `os` imported but unused
notify.py:2:8: F401 [*] `json` imported but unused
notify.py:5:28: B006 Do not use mutable data structures for argument defaults
notify.py:7:30: E711 Comparison to `None` should be `cond is None`
Found 4 errors.
[*] 2 fixable with the `--fix` option (2 hidden fixes can be enabled with the `--unsafe-fixes` option).
$ mypy --strict --no-error-summary --no-color-output notify.py
notify.py:5: error: Function is missing a type annotation  [no-untyped-def]
notify.py:13: error: Missing type arguments for generic type "dict"  [type-arg]
notify.py:14: error: Returning Any from function declared to return "int"  [no-any-return]

Ruff reports the unused imports, the mutable default argument (a classic Python bug: the same list is shared across calls, so reminders accumulate), and the comparison to None, which should use is. Mypy in strict mode reports the missing type annotations, the unparameterized dict, and that the function returns a value of unknown type (Any) where it promised an int. Neither tool catches that adding a string to a number will fail at run time here, because the dict is untyped; adding precise types, such as a dataclass for the shift, would let mypy catch that too. Tools are complementary to tests and reviews, not replacements.

Naming, comments and readability

Good names are the cheapest documentation. Names should say what something means in the domain (hours_by_program, not proc2 or data), be pronounceable and searchable, and follow the language's conventions, in Python those of PEP 8: snake_case for functions and variables, CapWords for classes, UPPER_CASE for constants. Functions should be named for what they do (send_reminder) and booleans for what they assert (is_full, has_parent_consent).

Comments should explain why, not what. 'Increment i' adds nothing; 'shifts are stored in UTC, so convert before comparing with the volunteer's local reminder time' saves the next reader an hour. When a comment explains what confusing code does, the better fix is usually to rename and extract until the code says it. Docstrings describe a function's contract: what it expects, what it returns and what it raises. Public interfaces deserve them; private helpers with clear names often do not.

Technical debt

Ward Cunningham introduced the technical debt metaphor in 1992: shipping code that is not quite right is like borrowing money. It can be a sensible way to deliver sooner, but the debt accrues interest, paid as extra effort on every later change, until it is repaid by refactoring. Martin Fowler's technical debt quadrant distinguishes debt that is deliberate or inadvertent and prudent or reckless. Deliberate and prudent: 'we must ship for the grant deadline and will clean up the report module next sprint.' Deliberate and reckless: 'we have no time for design.' Inadvertent and prudent: 'now we know how we should have done it.' Inadvertent and reckless: 'what is layering?'

Healthy teams make debt visible, as backlog items with an estimate of the interest it charges, and pay it down continuously rather than in rare 'rewrite' projects, which carry high risk. A practical rule from the Boy Scouts, popularized by Robert Martin, is to leave the code a little better than you found it: rename a confusing variable or extract a function as part of every change you make to that area.