# Refactoring

> Spot code smells, pin behaviour with characterization tests, and improve structure in small, safe, behaviour-preserving steps.

Software Engineer roadmap · Code that lasts · 17 min read · https://levelcode.org/roadmaps/software-engineer/refactoring

Refactoring is changing the structure of code without changing what it does. It is not a rewrite, not a feature, and not a two-week "cleanup sprint" — done well, it's a series of small steps, each of which leaves the code working, mixed continuously into everyday work. This lesson covers why and when to refactor, the smells that signal a problem, how to make legacy code safe to change, the core refactorings with Go examples, and how to fit refactoring into real delivery.

## What refactoring is — and isn't

Martin Fowler's definition has two parts: a change to the internal structure of software to make it easier to understand and cheaper to modify, **without changing its observable behaviour**. The second half is the discipline. If a commit renames a function *and* fixes a bug *and* adds a field, a reviewer can't tell which diff lines are the risky ones. Kent Beck's advice captures the rhythm:

> For each desired change, make the change easy (warning: this may be hard), then make the easy change.

So the workflow for most features is: refactor so the new feature has an obvious home, commit, then add the feature, commit. Two small reviewable diffs instead of one tangled one.

Refactor when:

- **You're about to change code** and its shape makes the change hard (preparatory refactoring).
- **You've just understood code** that took effort to read — encode that understanding in better names and structure before you lose it.
- **You see the same fix needed for the third time** (rule of three).

Don't refactor code you won't touch again just because it offends you, and don't refactor without tests.

## Code smells

A **smell** is a surface sign that suggests a deeper problem. It's a prompt to look closer, not a rule. The ones you'll meet most:

| Smell | What it looks like | Usual refactoring |
| --- | --- | --- |
| Long function | Scroll to read it; comments label its sections | Extract function |
| Long parameter list | `f(a, b, c, d, e, true)` | Introduce parameter object |
| Duplicated code | The same rule in several places | Extract function, move to one owner |
| Feature envy | A method uses another type's data more than its own | Move method |
| Data clumps | The same three fields always travel together | Extract type |
| Primitive obsession | Money as `float64`, email as `string`, status as magic strings | Introduce value type |
| Shotgun surgery | One change touches ten files | Move related code together |
| Divergent change | One file changes for many unrelated reasons | Split by responsibility |
| Switch on type, repeated | The same `switch` on a kind in several places | Replace with polymorphism |
| Speculative generality | Hooks, parameters and interfaces nobody uses | Inline, delete |

The [code smells note](https://levelcode.org/concepts/code-smells) is a quick reference. Notice how many smells are coupling and cohesion problems in disguise: shotgun surgery is low cohesion, feature envy is misplaced responsibility.

## Make it safe first: characterization tests

You can't refactor safely without tests that tell you when behaviour changed. Legacy code often has none, and you may not even know what the "correct" behaviour is. **Characterization tests** pin down what the code *currently* does — bugs included — so you can restructure with confidence:

1. Call the code with a representative input.
2. Write an assertion you know is wrong, run it, and read the actual output from the failure.
3. Change the assertion to the actual output. Repeat for edge cases.

```go
func TestLegacyInvoiceTotal_Characterization(t *testing.T) {
	tests := []struct {
		name  string
		items []LegacyItem
		want  string // whatever the code produces today
	}{
		{"single item", []LegacyItem{{"A", 2, 15000}}, "30000.00"},
		{"empty", nil, "0.00"},
		{"discount line", []LegacyItem{{"A", 1, 10000}, {"DISC", 1, -2000}}, "8000.00"},
		{"negative quantity", []LegacyItem{{"A", -1, 10000}}, "-10000.00"}, // a bug? pinned for now
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			if got := LegacyInvoiceTotal(tt.items); got != tt.want {
				t.Errorf("LegacyInvoiceTotal() = %q; want %q", got, tt.want)
			}
		})
	}
}
```

The "negative quantity" case is probably a bug, but refactoring isn't the moment to fix it. Pin it, finish the refactor, then fix the bug in a separate commit with a test that states the *intended* behaviour.

When code is too tangled to call in a test — it opens database connections in the middle, reads globals, or calls `time.Now()` — find a **seam**: a place where you can change behaviour without editing the code there. In Go, the usual seam is to extract the dependency into a parameter or an interface field, with the old behaviour as the default, so production callers are unaffected while tests can substitute a fake.

## The core refactorings

A small set of moves covers most day-to-day refactoring. Each is behaviour-preserving if done mechanically, and modern editors (gopls in VS Code or GoLand, the TypeScript language server) automate several: rename, extract function, extract variable, inline, move.

**Extract function.** Turn a fragment into a function named after *what* it does:

```go
// Before
func (s *Service) Notify(ctx context.Context, u User, msg string) error {
	// format the phone number
	phone := strings.ReplaceAll(u.Phone, " ", "")
	if strings.HasPrefix(phone, "0") {
		phone = "+62" + phone[1:]
	}
	return s.sms.Send(ctx, phone, msg)
}

// After
func (s *Service) Notify(ctx context.Context, u User, msg string) error {
	return s.sms.Send(ctx, normalizeIndonesianPhone(u.Phone), msg)
}

func normalizeIndonesianPhone(raw string) string {
	phone := strings.ReplaceAll(raw, " ", "")
	if strings.HasPrefix(phone, "0") {
		phone = "+62" + phone[1:]
	}
	return phone
}
```

The extracted function is now independently testable, and the comment became a name.

**Rename.** The cheapest, highest-value refactoring. When you understand what `data2` really holds, rename it everywhere — with the editor's rename, not find-and-replace.

**Introduce parameter object / extract type.** When `start, end time.Time` travel together through ten functions, make a `DateRange` with its own methods (`Contains`, `Overlaps`). Behaviour that was scattered across callers moves onto the type.

**Replace primitive with value type.** A `Money` type that stores minor units as `int64` and carries a currency removes a whole class of float-rounding and currency-mixing bugs. See [value objects](https://levelcode.org/concepts/value-objects).

**Move function.** Move a function to the type or package whose data it uses most (fixing feature envy).

**Replace conditional with polymorphism.** When the same `switch order.Type` appears in pricing, shipping and invoicing, give each type an implementation of one interface, as shown in the [SOLID lesson](https://levelcode.org/roadmaps/software-engineer/solid).

**Inline.** The reverse of extract. When an abstraction no longer earns its keep — a wrapper that only delegates, a helper with five flags — inline it back into its callers.

## Small steps, always green

The defining technique of refactoring is the **step size**. Each step should be small enough that if tests fail, you know exactly what caused it, and can undo it in seconds. Run the tests after every step; commit whenever they're green. If tests go red and you don't immediately see why, revert to the last green commit and take a smaller step — it's faster than debugging a half-finished restructure.

For changes too large for one sitting, use **parallel change** (also called expand–contract): add the new form alongside the old, migrate callers one at a time, then remove the old form. Every intermediate commit compiles and passes. Renaming a widely used function this way looks like: add `NewName` that calls `OldName`'s body, mark `OldName` deprecated with a `// Deprecated:` comment (Go tools flag uses), move callers over in several PRs, then delete `OldName`. The same idea applies to database schemas — see [expand–contract migrations](https://levelcode.org/concepts/expand-contract-migrations).

For replacing a whole subsystem, the **strangler fig** pattern routes a growing share of traffic from the old implementation to the new one behind a stable interface, until the old one can be deleted. It's how large rewrites ship without a big-bang cutover.

## Refactoring in a team

- **Separate refactoring commits from behaviour changes.** Reviewers can skim a pure rename and focus on the commit that changes logic. Say "no behaviour change" in the commit message, and mean it.
- **Leave the campsite a little cleaner** than you found it — but stay in scope. A bug fix PR that also reformats three unrelated packages is hard to review and harder to revert.
- **Don't ask permission to do your job well.** Small refactoring is part of building features, not a separate line item. Large restructures that take days *do* need a plan and buy-in; write a short [design doc](https://levelcode.org/roadmaps/software-engineer/design-docs) and deliver them incrementally.
- **Measure by outcome:** the next change in this area should be faster and less risky. If it isn't, the refactoring didn't help.

## Practice

The function below has several smells. List them, then describe a sequence of small refactoring steps (each leaving tests green) to improve it. Write the final code.

```go
func Report(orders []Order, from, to time.Time, currency string, includeRefunds bool) string {
	total := 0.0
	n := 0
	for _, o := range orders {
		if o.CreatedAt.After(from) && o.CreatedAt.Before(to) && o.Currency == currency {
			if o.Status == "refunded" && !includeRefunds {
				continue
			}
			total += o.Amount
			n++
		}
	}
	avg := 0.0
	if n > 0 {
		avg = total / float64(n)
	}
	return fmt.Sprintf("%d orders, total %.2f %s, average %.2f", n, total, currency, avg)
}
```

**Solution**

Smells: long parameter list with a **boolean flag**; a **data clump** (`from`, `to`); **primitive obsession** (money as `float64`, status as a magic string); mixed levels of abstraction (filtering, aggregation and formatting in one function).

Steps, running tests after each:

1. Write characterization tests covering in-range, out-of-range, other-currency, refunded with and without the flag, and zero matching orders.
2. Introduce `DateRange{From, To}` with a `Contains` method; replace the two parameters.
3. Introduce a constant `StatusRefunded` for the magic string.
4. Extract the filter into `matches(o Order) bool` on a filter type, which absorbs the currency and refund options.
5. Extract aggregation into `summarize([]Order) Summary`, leaving `Report` to format.
6. (Separate change, needs its own tests and review) move amounts from `float64` to integer minor units.

```go
const StatusRefunded = "refunded"

type DateRange struct{ From, To time.Time }

func (r DateRange) Contains(t time.Time) bool { return t.After(r.From) && t.Before(r.To) }

type ReportFilter struct {
	Period         DateRange
	Currency       string
	IncludeRefunds bool
}

func (f ReportFilter) Matches(o Order) bool {
	if !f.Period.Contains(o.CreatedAt) || o.Currency != f.Currency {
		return false
	}
	return f.IncludeRefunds || o.Status != StatusRefunded
}

type Summary struct {
	Count          int
	Total, Average float64
}

func Summarize(orders []Order, f ReportFilter) Summary {
	var s Summary
	for _, o := range orders {
		if f.Matches(o) {
			s.Count++
			s.Total += o.Amount
		}
	}
	if s.Count > 0 {
		s.Average = s.Total / float64(s.Count)
	}
	return s
}

func Report(orders []Order, f ReportFilter) string {
	s := Summarize(orders, f)
	return fmt.Sprintf("%d orders, total %.2f %s, average %.2f", s.Count, s.Total, f.Currency, s.Average)
}
```

Note that `Contains` keeps the original **exclusive** boundaries (`After`/`Before`). It's tempting to "fix" that to inclusive while you're there — don't, in a refactoring commit. If it's a bug, fix it separately with a test.

## Key takeaways

- Refactoring changes structure, never behaviour; keep it separate from feature and bug-fix commits.
- Make the change easy, then make the easy change: refactor in preparation for the feature you're about to build.
- Smells are prompts, not rules: long functions, data clumps, primitive obsession, feature envy, shotgun surgery.
- Pin legacy behaviour with characterization tests before changing it, and find seams to get it under test.
- Take small steps, run tests after each, commit when green, and revert rather than debug a broken restructure.
- Use parallel change and the strangler fig to evolve large code without a big-bang rewrite.

## Quiz

1. While refactoring, you discover that a function returns a negative total for negative quantities, which looks like a bug. What should you do?

   A. Fix it immediately as part of the refactoring commit
   B. Pin the current behaviour with a test, finish the refactoring, then fix the bug in a separate commit with a test for the intended behaviour
   C. Ignore it; refactoring never touches bugs
   D. Revert all your refactoring and file a ticket

   **Answer: B.** Mixing behaviour changes into a refactor makes both harder to review and revert. Finish the structural change with behaviour preserved, then fix the bug deliberately.

2. What is a characterization test?

   A. A test that documents what the code currently does, used as a safety net before changing it
   B. A test written before the code, as in TDD
   C. A load test that characterises performance
   D. A test of the user interface's appearance

   **Answer: A.** Characterization tests capture existing behaviour, including quirks, so you notice if a refactoring changes it.

3. A pricing method spends most of its lines reading fields of a Customer struct and very few of its own type. Which smell is this, and the usual fix?

   A. Shotgun surgery; split the method
   B. Speculative generality; delete the method
   C. Primitive obsession; introduce a value type
   D. Feature envy; move the method (or that part of it) to Customer

   **Answer: D.** Feature envy means behaviour lives away from the data it uses. Moving it to the type that owns the data raises cohesion.

4. You need to rename a function used in 300 places across many services' PRs. Which approach keeps every intermediate state working?

   A. A single PR that changes all 300 call sites at once
   B. Rename it and let compile errors show you where to fix
   C. Parallel change: add the new name alongside the old, migrate callers incrementally, then delete the old name
   D. Copy the function under the new name and leave both forever

   **Answer: C.** Expand–contract lets you migrate in small, reviewable steps where everything still compiles and passes, then contract by removing the old name once nothing uses it.