Refactoring a Legacy Testbench: A Code-Smell Catalog for DV

You inherited a testbench. It works—mostly. The regressions pass on a good day, the lead who wrote it left two tape-outs ago, and every time you add a feature you copy an existing test, tweak three lines, and pray. The env is two thousand lines. There is a run_phase you scroll through like a phone book. Somewhere in there a #100 delay holds the whole thing together and nobody knows why.

This is legacy code, and the uncomfortable truth is that most of us spend more time editing testbenches like this than writing clean ones from scratch. The software world has spent thirty years learning how to improve code you're afraid to touch. Four books in particular—Fowler's Refactoring, Martin's Clean Code, Feathers' Working Effectively with Legacy Code, and Hunt & Thomas's The Pragmatic Programmer—form the canon. This post translates their core ideas into the dialect of verification: a catalog of testbench code smells and the refactorings that cure them.

Note This post is the promised follow-up to Why Every DV Engineer Should Think Like a Software Engineer, which teased "code smells, refactoring patterns, technical debt" as a topic. Here's the payoff.

~22 min read · Intermediate · Assumes working UVM. Companion to Complexity Analysis: Why Your Testbench Is Slow and Memory Management: Surviving Long-Running Simulations in the Software Engineering for DV track.

A Testbench Is Legacy Code

Start with the most provocative definition in the field. Michael Feathers doesn't define legacy code as "old code" or "code I didn't write." He defines it by what it lacks:

"To me, legacy code is simply code without tests. Code without tests is bad code. It doesn't matter how well written it is; it doesn't matter how pretty or object-oriented or well-encapsulated it is. With tests, we can change the behavior of our code quickly and verifiably. Without them, we really don't know if our code is getting better or worse."

— Working Effectively with Legacy Code, Michael Feathers

Read that again with a DV hat on. Your testbench tests the DUT. But what tests the testbench? When you "improve" a scoreboard, what tells you that you didn't quietly disable a check? Nothing does. By Feathers' definition, almost every testbench is legacy code—it is the most safety-critical code in the project and it is itself untested.

This is the single most important idea in this post. Before you refactor a testbench, you need a safety net, and the safety net is a characterization test: a recording of what the system does right now, before you change anything.

Your characterization test is the golden regression

In application code, a characterization test pins down existing behavior so you notice if a refactor changes it. In verification, you already have the tool—you just have to use it deliberately:

// Characterization test for a testbench:
// 1. Run the full regression on a KNOWN-GOOD DUT (or a known-buggy one).
// 2. Capture the result fingerprint, not just pass/fail.
//
//   - final coverage numbers (functional + code)
//   - per-test error/warning counts
//   - scoreboard match/mismatch totals
//   - the set of seeds that fail (and how)
//
// 3. Save it as the golden baseline.
// 4. Refactor in small steps. After EACH step, re-run and diff against golden.
//    The refactor is correct only if the fingerprint is byte-for-byte identical.

Key takeaway A testbench refactor is "behavior-preserving" when the regression fingerprint—coverage, error counts, pass/fail seed set—is unchanged. If the numbers move, you didn't refactor; you changed behavior. Find out why before you commit.

That last point is Fowler's discipline. Refactoring is not "cleanup while I'm in here." It is a specific activity with a specific guarantee:

"Refactoring is a controlled technique for improving the design of an existing code base. Its essence is applying a series of small behavior-preserving transformations, each of which 'too small to be worth doing.' However the cumulative effect of each of these transformations is quite significant."

— Refactoring, Martin Fowler

Small steps. Green regression between each. Never refactor and add a feature in the same commit—if the fingerprint changes you won't know which change did it. With that safety net in place, let's open the patient up.

The patient: an APB register testbench

Every smell below is drawn from one running example—a small APB-style register-access environment of the kind every DV engineer has met. Here is the agent we inherited. It "works."

flowchart LR
    TEST[apb_test] --> ENV[apb_env]
    ENV --> AGT[apb_agent]
    AGT --> DRV[apb_driver]
    AGT --> MON[apb_monitor]
    AGT --> SEQR[apb_sequencer]
    ENV --> SB[apb_scoreboard]
    DRV -.drives.-> DUT[(APB DUT)]
    MON -.samples.-> DUT
    MON --> SB

    style ENV fill:#fef3c7,stroke:#f59e0b
    style SB fill:#dbeafe,stroke:#3b82f6

The shape is fine. The insides are where the smells live.

Smell 1: The God Environment

Open the inherited apb_env and you find that it does everything: builds components, parses plusargs, configures the DUT model, owns the scoreboard logic inline, prints the report, and decides test pass/fail. It is the textbook Large Class—what Fowler files under Bloaters.

"When a class is trying to do too much, it often shows up as too many instance variables. When a class has too many instance variables, duplicated code cannot be far behind."

— Refactoring, Martin Fowler

// BAD: the God Environment — one class, every responsibility
class apb_env extends uvm_env;
  uvm_component_utils(apb_env)

  apb_agent      agent;
  // ...plus 20 more handles for things an env should not own:
  int unsigned   num_txns;
  int unsigned   errors;
  int unsigned   warnings;
  bit            check_enable;
  bit            coverage_enable;
  string         test_mode;
  int unsigned   timeout_cycles;
  bit [31:0]     expected_mem [int];   // a shadow model, inline
  bit [31:0]     scoreboard_q [$];     // scoreboard state, inline

  function void build_phase(uvm_phase phase);
    super.build_phase(phase);
    agent = apb_agent::type_id::create("agent", this);

    // Config parsing lives here (it shouldn't)
    if (!$value$plusargs("TEST_MODE=%s", test_mode)) test_mode = "smoke";
    if (!$value$plusargs("TIMEOUT=%d", timeout_cycles)) timeout_cycles = 10000;
    check_enable    = !$test$plusargs("NO_CHECK");
    coverage_enable = !$test$plusargs("NO_COV");
  endfunction

  // Scoreboard logic, inline in the env (it shouldn't be)
  function void write(apb_transaction t);
    if (t.is_write) expected_mem[t.addr] = t.data;
    else if (expected_mem[t.addr] !== t.data) errors++;
  endfunction

  // Reporting, inline in the env (it shouldn't be)
  function void report_phase(uvm_phase phase);
    $display("ERRORS=%0d WARNINGS=%0d", errors, warnings);
  endfunction
endclass

The cure is Extract Class: give each responsibility its own home. Configuration becomes a uvm_object carried through the config_db. The scoreboard becomes a real uvm_scoreboard. The env goes back to its one job—assembling components.

// GOOD: config is its own object, set once, read anywhere
class apb_env_config extends uvm_object;
  uvm_object_utils(apb_env_config)
  string       test_mode      = "smoke";
  int unsigned timeout_cycles = 10000;
  bit          check_enable   = 1;
  bit          coverage_enable = 1;
  function new(string name = "apb_env_config"); super.new(name); endfunction
endclass

// GOOD: the env now only assembles. One responsibility.
class apb_env extends uvm_env;
  uvm_component_utils(apb_env)

  apb_agent        agent;
  apb_scoreboard   sb;      // scoreboard logic lives in the scoreboard
  apb_env_config   cfg;

  function void build_phase(uvm_phase phase);
    super.build_phase(phase);
    if (!uvm_config_db#(apb_env_config)::get(this, "", "cfg", cfg))
      cfg = apb_env_config::type_id::create("cfg");  // sane default
    agent = apb_agent::type_id::create("agent", this);
    sb    = apb_scoreboard::type_id::create("sb", this);
  endfunction

  function void connect_phase(uvm_phase phase);
    agent.mon.ap.connect(sb.analysis_export);
  endfunction
endclass

Tip The fastest way to find a God Environment is to count its instance variables and its responsibilities. If a one-sentence description of the class needs the word "and," it is doing too much.

Smell 2: Copy-Paste Sequences

The inherited suite has fifteen tests. Open any three and you'll find the same reset ritual, the same configuration writes, and the same teardown copied verbatim, with one or two lines changed. This is duplicated code, and the principle it violates has a name you already know.

"Every piece of knowledge must have a single, unambiguous, authoritative representation within a system."

— The Pragmatic Programmer (the DRY Principle), Hunt & Thomas

Duplication isn't just ugly—it's a liability. When the reset protocol changes, you must find and fix all fifteen copies, and you will miss one.

// BAD: every test re-implements the same setup by copy-paste
class write_read_test extends uvm_test;
  task run_phase(uvm_phase phase);
    phase.raise_objection(this);
    // --- copied reset ritual ---
    apb_reset_seq rst = apb_reset_seq::type_id::create("rst");
    rst.start(env.agent.seqr);
    // --- copied config writes ---
    apb_write_seq cfg0 = apb_write_seq::type_id::create("cfg0");
    cfg0.addr = 32'h0000_0000; cfg0.data = 32'h1; cfg0.start(env.agent.seqr);
    apb_write_seq cfg1 = apb_write_seq::type_id::create("cfg1");
    cfg1.addr = 32'h0000_0004; cfg1.data = 32'hF; cfg1.start(env.agent.seqr);
    // --- the actual test (the only unique part) ---
    apb_rw_seq rw = apb_rw_seq::type_id::create("rw");
    rw.start(env.agent.seqr);
    phase.raise_objection(this);  // <-- a real copy-paste bug: should be DROP
  endtask
endclass

Notice the bug hiding in the duplication: a copy-pasted raise_objection where a drop belongs. Duplication doesn't just multiply code—it multiplies defects. The cure is Extract Superclass for the shared scaffolding and Parameterize the part that varies.

// GOOD: shared setup lives once, in a base sequence
class apb_setup_seq extends uvm_sequence #(apb_transaction);
  uvm_object_utils(apb_setup_seq)
  function new(string name = "apb_setup_seq"); super.new(name); endfunction

  task body();
    apb_reset_seq rst;
    apb_write_seq cfg;
    uvm_do(rst)
    uvm_do_with(cfg, { addr == 32'h0; data == 32'h1; })
    uvm_do_with(cfg, { addr == 32'h4; data == 32'hF; })
  endtask
endclass

// GOOD: a base test owns the lifecycle; children supply only what varies
class apb_base_test extends uvm_test;
  uvm_component_utils(apb_base_test)
  apb_env env;

  function void build_phase(uvm_phase phase);
    super.build_phase(phase);
    env = apb_env::type_id::create("env", this);
  endfunction

  // Children override ONLY this hook
  virtual task run_main(uvm_phase phase); endtask

  task run_phase(uvm_phase phase);
    apb_setup_seq setup = apb_setup_seq::type_id::create("setup");
    phase.raise_objection(this);
    setup.start(env.agent.seqr);   // shared, single source of truth
    run_main(phase);               // the unique part
    phase.drop_objection(this);    // correct, in exactly one place
  endtask
endclass

// A concrete test is now three lines of intent
class write_read_test extends apb_base_test;
  uvm_component_utils(write_read_test)
  task run_main(uvm_phase phase);
    apb_rw_seq rw = apb_rw_seq::type_id::create("rw");
    rw.start(env.agent.seqr);
  endtask
endclass

Smell 3: Magic Numbers and Mystery Names

The inherited driver is a minefield of literals and one-letter names. What is 32'h4000_0010? What does a hold? You can't tell without archaeology.

"The name of a variable, function, or class, should answer all the big questions. It should tell you why it exists, what it does, and how it is used. If a name requires a comment, then the name does not reveal its intent."

— Clean Code, Robert C. Martin

// BAD: magic numbers and mystery names
task drive(apb_transaction t);
  if (t.addr == 32'h4000_0010) begin   // what is this register?
    vif.psel <= 1; vif.penable <= 0;
    #10;                                 // why 10?
    vif.penable <= 1;
    @(posedge vif.pclk);
    if (vif.prdata[3:0] == 4'h5) ...     // what does 5 mean?
  end
  for (int a = 0; a < 16; a++) ...       // 'a' is a loop counter?!
endtask

The cure is Replace Magic Number with Symbolic Constant plus honest naming. SystemVerilog gives you localparam, enum, and typedef to make intent executable.

// GOOD: names and constants carry the meaning
typedef enum bit [31:0] {
  REG_CTRL   = 32'h4000_0000,
  REG_STATUS = 32'h4000_0010,
  REG_DATA   = 32'h4000_0014
} apb_reg_addr_e;

typedef enum bit [3:0] {
  STATUS_IDLE  = 4'h0,
  STATUS_BUSY  = 4'h1,
  STATUS_READY = 4'h5
} apb_status_e;

localparam int SETUP_PHASE_NS = 10;   // APB SETUP-to-ACCESS, per spec §4.1

task drive(apb_transaction txn);
  if (txn.addr == REG_STATUS) begin
    start_setup_phase();
    #(SETUP_PHASE_NS);
    start_access_phase();
    @(posedge vif.pclk);
    if (apb_status_e'(vif.prdata[3:0]) == STATUS_READY) ...
  end
  for (int byte_idx = 0; byte_idx < APB_DATA_BYTES; byte_idx++) ...
endtask

Note The one comment worth keeping is the why: // APB SETUP-to-ACCESS, per spec §4.1. Clean Code's rule isn't "no comments"—it's that comments should explain intent the code can't, like a magic delay that exists because of a silicon erratum or a spec clause.

Smell 4: Primitive Obsession and Long Parameter Lists

The inherited driver's API takes everything as loose primitives—address, data, write-enable, byte-enable, a delay, a check flag—as six separate arguments. That is Primitive Obsession wearing a Long Parameter List, two more of Fowler's Bloaters. Primitives that always travel together are a data clump begging to become an object.

"You can shrink a lot of parameter lists, and simplify method calls, by combining the data clumps into objects... Groups of data that regularly appear together really ought to be made into their own object."

— Refactoring, Martin Fowler

// BAD: Primitive Obsession + Long Parameter List
task drive(bit [31:0] addr, bit [31:0] data, bit is_write,
           bit [3:0] byte_en, int delay_ns, bit check);
  // ...six positional args...
endtask

// Call site: quick — which argument is which?
drive(32'h4000_0010, 32'h0, 1, 4'hF, 10, 1);  // transpose two and it still compiles

The bug surface here is enormous: every call site can silently swap addr/data or is_write/check, and the compiler will never complain. The cure is Introduce Parameter Object—and in UVM you already have the perfect object: the transaction.

// GOOD: one typed, self-describing object (Introduce Parameter Object)
class apb_transaction extends uvm_sequence_item;
  rand apb_reg_addr_e addr;       // enum, not a bare bit[31:0] (see Smell 3)
  rand bit [31:0]     data;
  rand apb_dir_e      dir;        // READ / WRITE, not an ambiguous bit
  rand bit [3:0]      byte_en;
  // randomization constraints, convert2string, do_compare — all in one home
endclass

task drive(apb_transaction txn);  // one argument, impossible to transpose
  // ...
endtask

Now the type system catches mistakes the parameter list used to hide, and every consumer—driver, sequence, scoreboard—speaks the same vocabulary.

Smell 5: The 500-Line run_phase

The inherited monitor's run_phase is one unbroken task: it samples the bus, decodes the protocol, builds a transaction, updates coverage, prints debug, and writes the analysis port—four hundred lines deep. This is the Long Function, the most common smell of all.

"The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. Functions should do one thing. They should do it well. They should do it only."

— Clean Code, Robert C. Martin

// BAD: one task that does five jobs
task run_phase(uvm_phase phase);
  forever begin
    @(posedge vif.pclk);
    // ...50 lines sampling psel/penable/pready handshake...
    // ...80 lines decoding address/data/direction...
    // ...40 lines building the apb_transaction...
    // ...60 lines of coverage sampling inline...
    // ...30 lines of $display debug...
    // ...then finally ap.write(txn);
  end
endtask

The cure is Extract Function (well, Extract Task), each named for its single intent. The top-level run_phase becomes a readable table of contents.

// GOOD: run_phase reads like a summary; each step is one task
task run_phase(uvm_phase phase);
  apb_transaction txn;
  forever begin
    wait_for_transfer();
    txn = sample_transaction();
    if (cfg.coverage_enable) sample_coverage(txn);
    uvm_info("APB_MON", txn.convert2string(), UVM_HIGH)
    ap.write(txn);
  end
endtask

protected task wait_for_transfer();
  do @(posedge vif.pclk); while (!(vif.psel && vif.penable && vif.pready));
endtask

protected function apb_transaction sample_transaction();
  apb_transaction txn = apb_transaction::type_id::create("txn");
  txn.addr     = vif.paddr;
  txn.is_write = vif.pwrite;
  txn.data     = vif.pwrite ? vif.pwdata : vif.prdata;
  return txn;
endfunction

Each task now fits on a screen, names its purpose, and can be understood—and reviewed—on its own.

Smell 6: Inappropriate Intimacy

The inherited scoreboard doesn't receive transactions; it reaches in and takes them, dredging the monitor's internal queue directly. Fowler calls this a Coupler—Inappropriate Intimacy: two classes that know far too much about each other's private parts.

"Sometimes classes become far too intimate and spend too much time delving in each others' private parts. Over-intimate classes need to be broken up."

— Refactoring, Martin Fowler

// BAD: scoreboard reaches into the monitor's internals
class apb_scoreboard extends uvm_component;
  apb_monitor mon;   // a direct handle to another component
  task run_phase(uvm_phase phase);
    forever begin
      wait (mon.txn_q.size() > 0);     // reading a private queue
      apb_transaction t = mon.txn_q.pop_front();
      check(t);
    end
  endtask
endclass

This couples the scoreboard to the monitor's implementation. Change how the monitor stores transactions and the scoreboard breaks. The cure is the seam UVM already gives you: the TLM analysis port. The producer publishes; the consumer subscribes; neither knows the other's internals.

// GOOD: communicate through a defined interface (an analysis port)
class apb_scoreboard extends uvm_scoreboard;
  uvm_component_utils(apb_scoreboard)
  uvm_analysis_imp #(apb_transaction, apb_scoreboard) analysis_export;
  bit [31:0] shadow_mem [bit [31:0]];

  function new(string name, uvm_component parent);
    super.new(name, parent);
    analysis_export = new("analysis_export", this);
  endfunction

  // The monitor calls this via the port; no reaching in
  function void write(apb_transaction t);
    if (t.is_write) shadow_mem[t.addr] = t.data;
    else if (shadow_mem.exists(t.addr) && shadow_mem[t.addr] !== t.data)
      uvm_error("SB", $sformatf("Mismatch @%0h", t.addr))
  endfunction
endclass

This is Feathers' concept of a seam—"a place where you can alter behavior without editing in that place." The analysis port is a seam: you can swap the scoreboard, tap a second subscriber for coverage, or record transactions to a file, all without touching the monitor.

Smell 7: Shotgun Surgery

Add a single field to the protocol—say a parity bit—and watch the edit ripple outward: the transaction, the driver, the monitor, the scoreboard, the coverage model, and every sequence that constrains the field all need touching. Fowler calls this Shotgun Surgery, a Change Preventer: one logical change shotgunned across a dozen files. Its tell is that you can never make "a small change."

"Shotgun Surgery is when every time you make a kind of change, you have to make a lot of little edits to a lot of different classes... When the changes are all over the place, they are hard to find, and it's easy to miss an important one."

— Refactoring, Martin Fowler

// BAD: adding parity means hand-editing every layer
// apb_transaction.sv : add bit parity;
// apb_driver.sv      : vif.pparity <= txn.parity;
// apb_monitor.sv     : txn.parity = vif.pparity;
// apb_scoreboard.sv  : compare parity
// apb_coverage.sv    : coverpoint parity
// apb_*_seq.sv  x6   : constrain parity
//  -> miss one, and a whole field silently goes unchecked

The cure is a single source of truth. UVM's field automation and a register model (RAL) let you declare the field once and derive the rest, so a protocol change is a one-line edit, not a treasure hunt.

// GOOD: declare the field ONCE; copy/compare/print/pack come for free
class apb_transaction extends uvm_sequence_item;
  rand apb_reg_addr_e addr;
  rand bit [31:0]     data;
  rand bit            parity;            // <-- added in exactly one place
  uvm_object_utils_begin(apb_transaction)
    uvm_field_int(parity, UVM_ALL_ON)   // scoreboard compare + printing, automatic
  uvm_object_utils_end
endclass

Push it further with a uvm_reg model: register names, offsets, and field layout live in one generated model instead of scattered localparams, so the day the address map changes you regenerate rather than grep. Centralize the knowledge and the shotgun has nothing to scatter.

Smell 8: $display Spam

The inherited testbench debugs the way it was written at 2 a.m.: bare $display everywhere. You cannot turn it off, cannot filter it, and cannot tell a routine note from a fatal error in the log.

"The proper use of comments is to compensate for our failure to express ourselves in code. Every time you write a comment, you should grimace and feel the failure of your ability of expression."

— Clean Code, Robert C. Martin

The same spirit applies to log noise. A log that says everything says nothing. UVM gives you a structured reporting system with severity, verbosity, and IDs—use it.

// BAD: unfilterable, unleveled, unsearchable
$display("got txn");
$display("addr = %h data = %h", a, d);
$display("ERROR! mismatch");        // same prominence as the note above

// GOOD: severity + verbosity + a searchable ID
uvm_info("APB_DRV", $sformatf("driving %s", txn.convert2string()), UVM_HIGH)
uvm_info("APB_SB",  "comparison passed", UVM_MEDIUM)
uvm_error("APB_SB", $sformatf("mismatch @%0h: exp=%0h got=%0h",
                               txn.addr, exp, got))

Now +UVM_VERBOSITY=UVM_HIGH turns on the firehose for one debug run, the default keeps regressions quiet, the simulator counts your errors for you, and grep APB_SB finds every scoreboard message. This is the Pragmatic Programmer's broader lesson: the log is a tool, and tools should have controls.

Smell 9: Bypassing the Factory

The deepest smell is structural. The inherited code constructs everything with new(). To inject an error-driver for one test, you must edit the environment—and editing shared code to run one test is how testbenches rot.

// BAD: hard construction — the type is welded in place
class apb_agent extends uvm_agent;
  apb_driver drv;
  function void build_phase(uvm_phase phase);
    drv = new("drv", this);   // to use a different driver, edit THIS line
  endfunction
endclass

This is Feathers' central problem—you cannot change behavior without editing in place, because there is no seam. UVM's factory exists precisely to provide one.

"A seam is a place where you can alter behavior in your program without editing in that place. ... Every seam has an enabling point, a place where you can make the decision to use one behavior or another."

— Working Effectively with Legacy Code, Michael Feathers

// GOOD: construct through the factory — now there is a seam
class apb_agent extends uvm_agent;
  apb_driver drv;
  function void build_phase(uvm_phase phase);
    drv = apb_driver::type_id::create("drv", this);  // the seam
  endfunction
endclass

// The enabling point: a test changes behavior WITHOUT editing the agent
class error_injection_test extends apb_base_test;
  uvm_component_utils(error_injection_test)
  function void build_phase(uvm_phase phase);
    apb_driver::type_id::set_type_override(apb_error_driver::get_type());
    super.build_phase(phase);
  endfunction
endclass

create is the seam; set_type_override is the enabling point. The agent never changes; behavior does. This is also why the design-patterns work matters here—refactoring toward seams is refactoring toward patterns (the Factory, in this case). Joshua Kerievsky wrote a whole book on that path, Refactoring to Patterns; the Creational and Structural pattern posts are the destinations.

Smell 10: Tests That Can't Fail

Return to Feathers' thesis from a darker angle. He says legacy code is code without tests. The verification corollary is worse: a "test" that checks nothing is not neutral—it is a green light wired to a disconnected sensor. It actively lies.

// BAD: a 'test' that can never fail
class smoke_test extends apb_base_test;
  task run_main(uvm_phase phase);
    apb_rw_seq rw = apb_rw_seq::type_id::create("rw");
    rw.start(env.agent.seqr);
    // ...and that's all. No scoreboard connected, no assertions.
    // Passes forever — even on a DUT that returns garbage.
  endtask
endclass

This is the family of test smells—the dialect of code smell unique to verification:

• No oracle. The test drives stimulus but never compares against expected behavior. Coverage may climb while nothing is actually checked.
• Hardcoded golden values. if (rdata !== 32'hCAFE) bakes one expected answer into the test. It breaks on any legitimate change and hides real bugs behind a brittle constant.
• Flaky, non-deterministic tests. A fork ... join_any race whose pass/fail depends on simulator scheduling rather than the DUT. A test whose result you can't reproduce isn't measuring the design.
• Tests coupled to RTL internals. force tb.dut.fsm.state = ... or hierarchical peeking that shatters the moment the RTL is refactored—the testbench equivalent of Inappropriate Intimacy.

The cure is to give every test an oracle that is independent of the DUT: a scoreboard with a reference model, SystemVerilog Assertions for protocol legality, and functional coverage instead of hand-picked vectors. Replace timing races with event- or objection-based synchronization so the result depends on the design, not the scheduler.

Key takeaway A test that cannot fail is not a test—it is a liability with a green checkmark. Every test needs an oracle: a scoreboard, an assertion, or an expected value derived independently of the DUT.

Smell 11: Dead Code and Broken Windows

Finally, the smell everyone tolerates: the commented-out block from the 2019 bring-up, the if (0) guard, the task called from nowhere that no one dares delete. Individually harmless; collectively they rot the codebase's credibility.

"Don't leave 'broken windows' (bad designs, wrong decisions, or poor code) unrepaired. Fix each one as soon as it is discovered... One broken window, left unrepaired for any substantial length of time, instills in the inhabitants of the building a sense of abandonment."

— The Pragmatic Programmer, Hunt & Thomas

// BAD: the museum of abandoned debug
// if (0) begin
//   $display("temp debug from bringup");
//   ... 80 commented lines ...
// end
task legacy_drive(); / ... / endtask   // called from nowhere — but scary to remove

The cure (Fowler's Remove Dead Code) is the easiest in this whole catalog: delete it. Version control is your history—you lose nothing. Dead code costs every reader who has to wonder whether it matters, and each broken window left in place signals that decay is acceptable here. Fixing the small ones is how you keep the big ones from appearing.

The Refactored Result

Step back and look at what the four books bought us. The same testbench, made Easier To Change—the Pragmatic Programmer's ultimate test of good design.

flowchart TB
    subgraph Before["Before: tangled & untouchable"]
        BE[God env: config + scoreboard + reporting]
        BT[15 copy-paste tests]
        BM[500-line run_phase]
        BS[scoreboard reaches into monitor]
        BE -.- BT -.- BM -.- BS
    end

    subgraph After["After: seams everywhere"]
        AC[apb_env_config]
        AE[apb_env: assembles only]
        ABT[apb_base_test + setup_seq]
        ASB[scoreboard via analysis port]
        AF[factory overrides as seams]
        AE --> AC
        AE --> ASB
        ABT --> AE
        AE --> AF
    end

    Before ==>|"small, regression-green steps"| After

    style Before fill:#fee2e2,stroke:#ef4444
    style After fill:#d1fae5,stroke:#10b981

"Good design is easier to change than bad design. ... ETC is a value, not a rule. ... Ask yourself: does the thing I just did make the overall system easier or harder to change?"

— The Pragmatic Programmer, Hunt & Thomas

And you don't have to do it all at once. The Pragmatic Programmer's Boy Scout Rule (borrowed by Clean Code) is the sustainable path: leave the code a little cleaner than you found it. Renamed one mystery signal while fixing a bug? That's a refactor. Extracted one task from the monster run_phase? That's a refactor. Each is "too small to be worth doing," and the cumulative effect over a project is a testbench you're no longer afraid of.

When to Refactor—and When to Stop

A catalog of cures invites a dangerous reflex: refactor everything, all the time. That is its own failure mode. The books are equally clear about when.

The Rule of Three (Kent Beck, via Fowler): the first time you do something, just do it. The second time you do something similar, wince at the duplication but do it anyway. The third time, refactor. Two copies of a reset sequence is a coincidence; three is a pattern asking for a base sequence. Refactor when you're already there. The cheapest refactoring rides along with a change you were making anyway—the campsite you're already standing in. Refactor when you add a feature, when you fix a bug, when you're reverse-engineering a tangled run_phase to understand it. The "big refactoring project," funded separately and scheduled against features, almost never survives contact with a tape-out. Small, opportunistic, regression-green steps do.

This is the economics of technical debt, a metaphor worth taking literally:

"Shipping first-time code is like going into debt. A little debt speeds development so long as it is paid back promptly with a rewrite... The danger occurs when the debt is not repaid. Every minute spent on not-quite-right code counts as interest on that debt."

— Ward Cunningham

Fowler sorts debt along two axes—deliberate vs. inadvertent and reckless vs. prudent. A testbench shortcut taken knowingly under tape-out pressure, with a ticket filed to fix it, is deliberate and prudent: legitimate. The killer is inadvertent debt—the smells you don't even recognize as smells. This entire catalog is a debt detector: it gives names to the interest you're paying. In a long regression, that interest is concrete—a god-class monitor that's hard to change is also, often, the one that's slow to run or leaks memory across a long simulation.

When to stop is the same question inverted. Stop when the code is Easier To Change than it was and the next change is no longer painful. Don't gold-plate. A scoreboard clean enough to extend in five minutes does not need a plugin architecture—building one is Speculative Generality, a smell in its own right. ETC is the test in both directions: refactor until changing is easy, then leave it alone.

The Catalog at a Glance

Smell	Book / source	Named refactoring	DV cure
God Environment	Fowler — Large Class	Extract Class	config object + real scoreboard; env only assembles
Copy-Paste Sequences	Pragmatic — DRY	Extract Superclass / Parameterize Method	base test + base sequence
Magic Numbers / Mystery Names	Martin — naming	Replace Magic Literal with Symbolic Constant	enum / typedef / localparam, honest names
Primitive Obsession / Long Param List	Fowler — Bloaters	Introduce Parameter Object	one typed transaction object
500-line run_phase	Martin — small functions	Extract Function	intention-named tasks
Inappropriate Intimacy	Fowler — Couplers	Hide Delegate (use a seam)	TLM analysis port
Shotgun Surgery	Fowler — Change Preventers	Move Field / centralize	register model + single txn definition
$display Spam	Martin / Pragmatic	Replace with reporting API	uvm_info/uvm_error + verbosity + IDs
Bypassing the Factory	Feathers — seams	Introduce Seam	type_id::create + overrides
Tests That Can't Fail	Feathers — code without tests	Add an Oracle	scoreboard / SVA / coverage
Dead Code	Pragmatic — Broken Windows	Remove Dead Code	delete it; trust version control

Common Mistakes

1. Refactoring Without a Regression Net

// BAD: "I'll just clean this up real quick" — no baseline captured.
// You change behavior and never notice the check you disabled.

Never refactor a testbench you haven't fingerprinted first (coverage, error counts, failing-seed set). Without the baseline you are not refactoring—you are gambling. Capture golden, then change.

2. The Big-Bang Rewrite

The most expensive mistake is deciding the testbench is hopeless and rewriting it from scratch. You throw away years of accumulated bug-finding knowledge encoded in those ugly sequences. Feathers' entire book exists to argue the opposite: change legacy code incrementally, behind tests. Strangle the old bench one seam at a time.

3. Mixing Refactor and Feature in One Commit

# BAD: a single commit that both renames things AND adds a new check.
# When a seed starts failing, you cannot tell which change broke it.
git commit -m "refactor scoreboard + add parity check + rename signals"

# GOOD: separate, verifiable steps
git commit -m "refactor: extract scoreboard from env (regression identical)"
git commit -m "feat: add parity check to scoreboard"

Refactor commits keep the fingerprint identical. Feature commits change it on purpose. Keep them apart so git bisect can do its job.

4. "Clean" That Adds Indirection Nobody Needs

Refactoring is not about adding layers. A wrapper around a wrapper around a config_db lookup is not cleaner—it's Speculative Generality, another of Fowler's smells. Apply YAGNI: extract a class when a responsibility actually needs a home, not because a diagram looks tidier.

Key Takeaways

• A testbench is legacy code—it's untested code that tests. Treat it with the same respect (and fear) as any code without tests.
• Your characterization test is the golden regression. Fingerprint coverage, error counts, and failing seeds before you touch anything; a refactor is correct only when the fingerprint is unchanged.
• The God Environment → Extract Class: config to a config_db object, scoreboard to a uvm_scoreboard, env back to assembling.
• Copy-paste sequences → DRY via a base test/sequence; duplication multiplies defects, not just lines.
• Magic numbers and mystery names → symbolic constants, enums, honest names; keep only the why comments.
• Primitive Obsession / long parameter lists → Introduce Parameter Object: pass one typed transaction, not a fistful of loose primitives the compiler can't check.
• The 500-line run_phase → Extract Task; make the top level a table of contents.
• Inappropriate Intimacy → communicate through analysis ports (seams), never reach into another component's internals.
• Shotgun Surgery → a single source of truth (register model, one transaction definition); a protocol field should be added in exactly one place.
• $display spam → uvm_info/uvm_error with verbosity and IDs; a controllable log is a tool.
• Bypassing the factory → type_id::create is the seam, set_type_override is the enabling point; change behavior without editing shared code.
• Tests that can't fail → give every test an oracle (scoreboard, SVA, independent expected value); coverage without checking is false confidence.
• Dead code → delete it; version control is your history, and broken windows invite more.
• Apply the Rule of Three and refactor when you're already there; pay down technical debt deliberately, and stop once the code is Easier To Change—don't gold-plate.

Further Reading

• Working Effectively with Legacy Code — Michael Feathers (seams, characterization tests, the definition of legacy code)
• Refactoring, 2nd ed. — Martin Fowler (the smell taxonomy and the discipline)
• Clean Code — Robert C. Martin (naming, functions, and Chapter 17, "Smells and Heuristics")
• The Pragmatic Programmer, 20th Anniversary ed. — Hunt & Thomas (DRY, ETC, Broken Windows, the Boy Scout Rule)
• Refactoring to Patterns — Joshua Kerievsky (the bridge between this post and the Design Patterns series)
• Test Driven Development: By Example — Kent Beck (the Rule of Three and the red-green-refactor cycle)
• Ward Cunningham — the original "technical debt" metaphor (the WyCash report; see also Fowler's "Technical Debt Quadrant")

Interview Corner

Q: What makes a testbench "legacy code," and why does it matter?

A: By Michael Feathers' definition, legacy code is simply code without tests—and a testbench, despite being test code, is almost never tested itself. It matters because without a safety net you can't refactor safely: you have no way to know whether a change preserved behavior. The practical answer is to use the regression as a characterization test—capture a fingerprint (coverage, error counts, failing seeds) before changing anything, and require it to stay identical across each refactoring step.

Q: How do you refactor a testbench without risking the project's sign-off quality?

A: In small, behavior-preserving steps, each verified against a golden regression baseline, never mixing a refactor with a feature in the same commit. "Behavior-preserving" for a testbench means the regression fingerprint is unchanged. If coverage or error counts move, the change wasn't a refactor and needs investigation before it's committed.

Q: What is a "seam," and where do they already exist in UVM?

A: A seam is a place where you can alter behavior without editing in that place; its enabling point is where you choose which behavior to use. UVM is full of them: the factory (type_id::create is the seam, set_type_override/instance overrides are the enabling points), the config_db, and TLM analysis ports. Recognizing these as seams is what lets you change behavior per-test without editing shared environment code—the antidote to testbench rot.

Q: A protocol gets a new field and you have to edit ten files to support it. What smell is that, and how do you fix it?

A: That's Shotgun Surgery, one of Fowler's Change Preventers—a single logical change scattered across many classes, where it's easy to miss one and silently leave the field unchecked. The fix is a single source of truth: declare the field once on the transaction and lean on UVM field automation for copy/compare/print, and keep register names, offsets, and layout in a generated register model (RAL) rather than scattered localparam`s. Then a protocol change is a one-line edit, not a treasure hunt.

Q: How do you decide when to refactor versus leaving code alone?

A: Use the Rule of Three—refactor on the third occurrence of duplication, not the first—and refactor opportunistically when you're already changing the code for a feature or bug, rather than as a separate project that competes with the schedule. Treat it as paying down technical debt: deliberate, prudent debt taken under deadline pressure is fine if you track it, but inadvertent debt compounds. Stop when the code is Easier To Change than it was; refactoring past that point into speculative generality is itself a smell.

---

Previous: Structural Patterns: Adapter, Facade, and Composite in Verification | Next: Behavioral Patterns (Coming Soon) Return to Software Engineering for DV