Refactor code safely: name variables, split functions

How do you name variables so other people can read them? A name should say what the variable holds. But renaming things in old code is only safe when tests prove the behavior didn't change. Have you ever tidied code "to make it nicer" and broken something that worked? Let's clean up one file together.

What did Khoa dislike about the file?

In the previous post, the pull request fixing the "order 1044 adds up to 70 đồng" bug in the order system of Mộc was approved and merged. Mộc is a small Vietnamese coffee shop that sells online; the shop and Khoa, the reviewer, are made up for this series. The code and every output in this post are real.

The approval came with a note: "Next time you touch server.ts, let's clean it up. That file takes a while to read: names like rawOrders, orders, body don't say what they hold, and one function reads the file, normalizes it and handles HTTP all at once."

Here is the whole file. It's a small HTTP server built on Node.js's built-in node:http module. At startup it reads data/orders.json, turns every string price like "35.000" into a number with normalizeOrder, and serves the list of orders at the path /orders:

import { readFileSync } from "node:fs";
import { createServer } from "node:http";

import { listOrders, normalizeOrder, type RawOrder } from "./orders.ts";

const dataFile = new URL("../data/orders.json", import.meta.url);
// Normalize as soon as the file is read: from here on, prices are always numbers.
const rawOrders: RawOrder[] = JSON.parse(readFileSync(dataFile, "utf8"));
const orders = rawOrders.map(normalizeOrder);

export const server = createServer((req, res) => {
  if (req.url === "/orders") {
    try {
      const body = JSON.stringify(listOrders(orders), null, 2);
      res.writeHead(200, { "content-type": "application/json; charset=utf-8" }).end(body);
    } catch (error) {
      console.error(error);
      res.writeHead(500, { "content-type": "text/plain; charset=utf-8" }).end("Server error");
    }
    return;
  }
  res.writeHead(404).end();
});

const port = Number(process.env.PORT ?? 3000);
server.listen(port, () => console.log(`Mộc orders: http://localhost:${port}/orders`));

Run it and open http://localhost:3000/orders, and you get the four orders from the morning of 3 November. Each has an id, the customer name in capitals and a total:

[
  { "id": 1041, "customer": "NGUYỄN AN", "total": 83000 },
  { "id": 1042, "customer": "TRẦN BÌNH", "total": 32000 },
  { "id": 1043, "customer": "WALK-IN CUSTOMER", "total": 25000 },
  { "id": 1044, "customer": "LÊ CHI", "total": 70000 }
]

I put each order on one line to save space; the real server prints one field per line. The file is only 25 lines, so you may wonder why it needs cleaning. To me the reason is that this file is the entry point of the whole system. Everyone new to the project opens it first, and today they have to read every line to learn that it does three different jobs. More features are coming, and a 25-line file becomes 200 lines if nobody splits it up.

What is refactoring, and how is it different from fixing a bug?

Martin Fowler, who wrote the well-known book on the subject, defines refactoring as changing the internal structure of code without changing its external behavior. The second half is the hard part. After refactoring, the server must return exactly the same bytes as before, the same status codes, the same headers, and still stop when it meets a bad price.

How do you know behavior didn't change? The system already has 15 tests written with Vitest, from the post on the first test and the code review post. But all 15 test orders.ts, the functions that read prices and compute totals. No test imports server.ts. If I clean up server.ts and accidentally change the path /orders to /orders/, all 15 tests stay green.

So the first job isn't renaming. The first job is writing a test for the thing I'm about to change.

What do you test when you don't know what "correct" is?

The kind of test I need here is called a characterization test. It doesn't ask whether the code is right. It only records what the code does now, so that after the change I can compare with before. If the old code has a strange behavior, a characterization test records the strange behavior too, because a refactoring is not the moment to fix it.

There's one obstacle. server.ts calls server.listen as soon as it's imported, on port 3000. If you have the server running on that port in another terminal, the test fails because the port is taken. I avoid this by setting the PORT environment variable to 0 before importing. The Node.js documentation says that when the port is 0, the operating system picks a free one.

// Characterization test: captures what server.ts does RIGHT NOW, before the cleanup.
// It doesn't ask "is the code correct?", only "after the cleanup, does the server answer exactly as before?".
import type { AddressInfo } from "node:net";
import { afterAll, beforeAll, expect, test } from "vitest";

let baseUrl = "";
let close = () => {};

beforeAll(async () => {
  process.env.PORT = "0"; // port 0: the operating system picks a free port, so we don't clash with a server running on 3000
  const { server } = await import("./server.ts");
  if (!server.listening) await new Promise((resolve) => server.once("listening", resolve));
  baseUrl = `http://localhost:${(server.address() as AddressInfo).port}`;
  close = () => server.close();
});

afterAll(() => close());

test("GET /orders returns the order list exactly as before the cleanup", async () => {
  const response = await fetch(`${baseUrl}/orders`);
  expect(response.status).toBe(200);
  expect(response.headers.get("content-type")).toBe("application/json; charset=utf-8");
  expect(await response.json()).toEqual([
    { id: 1041, customer: "NGUYỄN AN", total: 83000 },
    { id: 1042, customer: "TRẦN BÌNH", total: 32000 },
    { id: 1043, customer: "WALK-IN CUSTOMER", total: 25000 },
    { id: 1044, customer: "LÊ CHI", total: 70000 },
  ]);
});

test("any other path returns 404", async () => {
  const response = await fetch(`${baseUrl}/nothing-here`);
  expect(response.status).toBe(404);
});

I use import() inside beforeAll rather than an import line at the top of the file, because a top-level import runs before anything else, which means before PORT has been set to 0. The four orders in the test are the real ones from data/orders.json, copied from the output above.

 Test Files  2 passed (2)
      Tests  17 passed (17)

While the tests run, the server's log line also comes through. Add --reporter=verbose to see it:

stdout | src/server.test.ts > GET /orders returns the order list exactly as before the cleanup
Mộc orders: http://localhost:0/orders

The log prints the port I asked for, not the real port the operating system chose. That's a small bug, but fixing it changes behavior, so it doesn't belong in this refactoring. I made a note to fix it in a separate commit.

The first commit on the branch refactor/clean-server-ts contains only this test file. That way anyone reading the history sees the test came first, and can check out that commit to confirm the test is green on the old code.

How do you name variables so the next person understands?

Now the renaming. I go through each name Khoa mentioned, and for each one I ask myself: what does a reader need to know about this value that the current name doesn't say?

body only says where the value goes in an HTTP response: the body part. It doesn't say what the body contains. The reader has to look at the right-hand side to learn it's the order list turned into a JSON string. I renamed it orderListJson. It's longer, but by the end of the line .end(orderListJson) you know what the server is sending.

The pair rawOrders and orders is harder, because they're the same data in two states. "Raw" means unprocessed, but raw in what sense? I only found out by reading the RawOrder type: "an order as stored in the file, where the price may be a string". I renamed rawOrders to ordersInFile, because that name carries its origin. orders I kept, and the splitting step below shows why that works.

dataFile isn't wrong, but the system will soon have other files, such as a menu file or a customers file. I renamed it ORDERS_FILE. The capital letters are a common JavaScript convention for a configuration constant at the top of a module, telling the reader this value stays fixed while the program runs.

req and res are very familiar abbreviations in Node.js code, so I hesitated for a while. In the end I changed them to request and response, because nothing else in the project abbreviates and a newcomer shouldn't have to guess. If your team already uses req/res, keeping them is fine. Being consistent across the whole project matters more than picking the one right style.

Finally, orders.ts has a small helper called display. "Display" doesn't say what it does. It wraps a string in quotes and leaves a number alone, so that an error message can tell the string "35" from the number 35. I renamed it quoteIfText: "add quotes if it's text". It also starts with a verb, so the reader knows it does one thing rather than holds a value.

If I boil this down to a few questions to ask when naming something, I use these four:

1. Reading just the name, without looking at the right-hand side, can I guess what it holds?
2. If the same data appears in two states, do the two names say which is which?
3. Does the function name start with a verb, and is that verb really what the function does?
4. Is the new name in the same language and the same style as the rest of the project?

What happens when a "while I'm here" rename goes wrong?

While renaming, I started looking at the keys in the JSON the server returns. customer and total felt vague. Customer what, the name or the id? Total in which currency? Since I was already renaming things, why not make them customerName and totalVnd? I changed the two keys in listOrders and ran the tests:

 FAIL  src/orders.test.ts > the real data in data/orders.json > totals of all four orders
AssertionError: expected [ Array(4) ] to deeply equal [ { id: 1041, total: 83000 }, …(3) ]
- Expected
+ Received

  [
    {
      "id": 1041,
-     "total": 83000,
+     "total": undefined,
    },

 FAIL  src/server.test.ts > GET /orders returns the order list exactly as before the cleanup
- Expected
+ Received

  [
    {
-     "customer": "NGUYỄN AN",
+     "customerName": "NGUYỄN AN",
      "id": 1041,
-     "total": 83000,
+     "totalVnd": 83000,
    },

I trimmed the output, because the rest repeats for the other orders. Two tests went red, and both were right. Renaming a variable inside a function is invisible to everyone outside. But customer and total are the field names in the JSON the server returns, and the shop's order management page reads exactly those two names. Changing them changes the outside behavior, in other words the API, and that step needs a heads-up to whoever maintains the management page, a transition period, and a pull request of its own.

I reverted the two lines. The boundary this red test drew is clear: names inside the code are free to change, but a name that other code depends on is part of the behavior. Without server.test.ts, I'd have pushed that change and the shop's page would have shown an empty total column.

After the revert, all 17 tests were green again and I committed the renaming step. This commit only renames; no line of code moves. Here is the whole diff of server.ts (the orders.ts part is just display becoming quoteIfText in three places):

-const dataFile = new URL("../data/orders.json", import.meta.url);
+const ORDERS_FILE = new URL("../data/orders.json", import.meta.url);
 // Normalize as soon as the file is read: from here on, prices are always numbers.
-const rawOrders: RawOrder[] = JSON.parse(readFileSync(dataFile, "utf8"));
-const orders = rawOrders.map(normalizeOrder);
+const ordersInFile: RawOrder[] = JSON.parse(readFileSync(ORDERS_FILE, "utf8"));
+const orders = ordersInFile.map(normalizeOrder);

-export const server = createServer((req, res) => {
-  if (req.url === "/orders") {
+export const server = createServer((request, response) => {
+  if (request.url === "/orders") {
     try {
-      const body = JSON.stringify(listOrders(orders), null, 2);
-      res.writeHead(200, { "content-type": "application/json; charset=utf-8" }).end(body);
+      const orderListJson = JSON.stringify(listOrders(orders), null, 2);
+      response.writeHead(200, { "content-type": "application/json; charset=utf-8" }).end(orderListJson);
     } catch (error) {
       console.error(error);
-      res.writeHead(500, { "content-type": "text/plain; charset=utf-8" }).end("Server error");
+      response.writeHead(500, { "content-type": "text/plain; charset=utf-8" }).end("Server error");
     }
     return;
   }
-  res.writeHead(404).end();
+  response.writeHead(404).end();

I split renaming and moving code into two commits because a reviewer reads them in two different ways. A commit that only renames is very quick to read, since each red line has exactly one matching green line. Mixed with moved code, the diff becomes one big block that the reader has to compare character by character.

How do you split a function: ask who needs to change this part

The second half of Khoa's note was that server.ts does too many things. I count three: reading and normalizing the file, answering HTTP requests, and starting up (choosing the file, choosing the port, opening the port). Those three change for three different reasons. When the shop moves from a JSON file to a database, only the first changes. When a new path is added, only the second changes. When the server goes live on a different port, only the third changes.

I put each job in its own place. Reading the file goes into order-file.ts:

import { readFileSync } from "node:fs";

import { normalizeOrder, type Order, type RawOrder } from "./orders.ts";

// Reads the orders file and normalizes it right away: from here on, prices are always numbers.
// One bad price makes the whole function throw, so the server stops instead of skipping the order.
export function loadOrders(file: URL): Order[] {
  const ordersInFile: RawOrder[] = JSON.parse(readFileSync(file, "utf8"));
  return ordersInFile.map(normalizeOrder);
}

That's why orders didn't need renaming. Before, orders "in the file" and orders "normalized" lived side by side in the same scope, so the names had to tell them apart. Now ordersInFile lives only inside loadOrders, and everywhere else in the project sees only normalized orders. When a scope holds only one kind of order, the short name orders is clear enough. Splitting a function often makes naming easier in exactly this way.

Answering requests goes into order-server.ts. The function createOrderServer receives the orders as a parameter instead of reading the file itself, and returns the server without opening a port:

import { createServer, type Server, type ServerResponse } from "node:http";

import { listOrders, type Order } from "./orders.ts";

const JSON_HEADERS = { "content-type": "application/json; charset=utf-8" };
const TEXT_HEADERS = { "content-type": "text/plain; charset=utf-8" };

// HTTP server for the order list. Takes already-normalized orders, doesn't read files, doesn't open a port itself.
export function createOrderServer(orders: Order[]): Server {
  return createServer((request, response) => {
    if (request.url === "/orders") return sendOrderList(response, orders);
    response.writeHead(404).end();
  });
}

function sendOrderList(response: ServerResponse, orders: Order[]): void {
  try {
    const orderListJson = JSON.stringify(listOrders(orders), null, 2);
    response.writeHead(200, JSON_HEADERS).end(orderListJson);
  } catch (error) {
    console.error(error);
    response.writeHead(500, TEXT_HEADERS).end("Server error");
  }
}

The handling of /orders became its own function, sendOrderList, so the routing is just two lines. Reading createOrderServer tells you at a glance which paths the server has, without wading through a try/catch block. To add a new path, you add one if line and one new send... function.

server.ts keeps only the startup:

// Entry point: read the data, build the server, open the port. No other logic lives here.
import { loadOrders } from "./order-file.ts";
import { createOrderServer } from "./order-server.ts";

const ORDERS_FILE = new URL("../data/orders.json", import.meta.url);
const port = Number(process.env.PORT ?? 3000);

export const server = createOrderServer(loadOrders(ORDERS_FILE));
server.listen(port, () => console.log(`Mộc orders: http://localhost:${port}/orders`));

I kept export const server under exactly the same name, because server.test.ts imports it. That's the condition for the characterization test to run on both the old code and the new without changing a line. If you have to edit a test to make it pass after a refactoring, it no longer proves anything.

 Test Files  2 passed (2)
      Tests  17 passed (17)

I also ran the real server once more and called /orders. The result matched the list at the top of this post exactly, from the order of the fields to the capitalized names.

Which new tests become possible after the split?

This is my favorite part of the cleanup. Before the split, testing the server meant running it against the real data/orders.json. To try an order with a bad price, I'd have had to edit the real data file. Now both functions take their data from outside, so I can feed in whatever I like.

The test for loadOrders writes a temporary file, so it can try any data without touching the real one. The second test locks in a decision from comment 6 of the previous post: the server must stop on a bad price rather than skip the order:

import { mkdtempSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { pathToFileURL } from "node:url";
import { expect, test } from "vitest";

import { loadOrders } from "./order-file.ts";

// Writes a temporary orders file, so loadOrders can be tried with any data without touching data/orders.json.
function tempOrdersFile(orders: unknown): URL {
  const file = join(mkdtempSync(join(tmpdir(), "orders-")), "orders.json");
  writeFileSync(file, JSON.stringify(orders));
  return pathToFileURL(file);
}

test("reads the file and turns string prices into numbers", () => {
  const file = tempOrdersFile([
    { id: 2001, createdAt: "2026-11-13T08:00:00+07:00", source: "old-pos", items: [{ name: "Bac xiu", price: "32.000", quantity: 1 }] },
  ]);
  expect(loadOrders(file)[0].items[0].price).toBe(32000);
});

test("one bad price makes the whole file rejected, so the server won't start with broken data", () => {
  const file = tempOrdersFile([
    { id: 2002, createdAt: "2026-11-13T08:05:00+07:00", source: "old-pos", items: [{ name: "Salt coffee", price: 35, quantity: 2 }] },
  ]);
  expect(() => loadOrders(file)).toThrow("Invalid price: 35");
});

The tests for createOrderServer build a server with exactly the orders each test needs. The third one exercises the catch branch that returns a 500 error, a branch you could never reach with the real data before:

import type { AddressInfo } from "node:net";
import { expect, test } from "vitest";

import { createOrderServer } from "./order-server.ts";
import type { Order } from "./orders.ts";

// Builds a server with exactly the orders each test needs, opens it on a free port, sends one request, then closes it.
async function get(orders: Order[], path: string): Promise<Response> {
  const server = createOrderServer(orders);
  await new Promise<void>((resolve) => server.listen(0, resolve));
  try {
    return await fetch(`http://localhost:${(server.address() as AddressInfo).port}${path}`);
  } finally {
    server.close();
  }
}

const counterOrder: Order = {
  id: 2003,
  createdAt: "2026-11-13T08:10:00+07:00",
  source: "old-pos",
  items: [{ name: "Black coffee", price: 25000, quantity: 2 }],
};

test("no orders returns an empty list", async () => {
  const response = await get([], "/orders");
  expect(await response.json()).toEqual([]);
});

test("a counter order with no customer still shows up", async () => {
  const response = await get([counterOrder], "/orders");
  expect(await response.json()).toEqual([{ id: 2003, customer: "WALK-IN CUSTOMER", total: 50000 }]);
});

test("broken customer data returns 500 and doesn't crash the server", async () => {
  // The customer has a phone number but no name: customerLabel calls toUpperCase on undefined.
  const brokenOrder = { ...counterOrder, customer: { phone: "0901000009" } } as unknown as Order;
  const response = await get([brokenOrder], "/orders");
  expect(response.status).toBe(500);
  expect(await response.text()).toBe("Server error");
});

 Test Files  4 passed (4)
      Tests  22 passed (22)

New tests need checking too. As in the Vitest post, I deliberately broke the code to see whether the tests go red. I changed loadOrders to return the data from the file without normalizing it, which removes the most important line of the function:

 × reads the file and turns string prices into numbers 7ms
 × one bad price makes the whole file rejected, so the server won't start with broken data 3ms
AssertionError: expected '32.000' to be 32000 // Object.is equality
AssertionError: expected [Function] to throw an error

Both tests went red, each for its own reason. I put the line back and committed.

Four commits, and why so small

After the cleanup, the history of the branch looks like this:

1d0939f test: add tests for loadOrders and createOrderServer
b44f8cd refactor: move file reading and server setup out of server.ts
883620b refactor: name variables for what they hold
d3d58fe test: capture the behavior of server.ts before cleaning it up

The tests are green on every commit. If someone later notices the server answering differently, git bisect or checking out each commit in turn finds the step that caused it. The two refactoring commits carry the refactor: prefix so the reviewer knows up front that they don't need to look for behavior changes in them, only to check whether the code reads better.

You'll also notice the "localhost:0" log bug isn't fixed yet. It will be a separate fix: commit, small and easy to read, after the refactoring branch is approved. Mixing a behavior change into the middle of refactoring commits is the fastest way to get a reviewer to miss it.

Where do you stop refactoring?

There's more I could clean up. listOrders still builds the response shape. customerLabel still crashes when a customer has no name, and the new test only proves that the server doesn't crash with it. I stopped here because Khoa's note was about server.ts, and this pull request has done exactly that. The bigger a refactoring pull request is, the harder it is to review, and the more likely someone says "I'll read it next week".

If you want to redo everything on your machine, you need the project folder with data/orders.json and src/orders.ts from the earlier posts, then create the new files exactly as shown here: server.test.ts, order-file.ts, order-server.ts and the two new test files. You need Node.js 22.18 or later to run .ts files directly, and Vitest installed with npm install -D vitest.

What happens when orders start arriving from somewhere else?

The code is now easier to read, and because createOrderServer takes its orders as a parameter, adding a new path is no longer scary. Right then, the owner of Mộc messaged: the shop's website will soon send orders straight to this server with a POST request, instead of waiting for someone to export a JSON file every morning.

The first question that came to my mind was what the data sent in would look like. The old machine once sent prices like "35.000". The new website might send a missing field, a quantity as a string or an empty array. parsePrice only checks the price, and nobody checks the rest of the order. Validating a whole incoming order without writing dozens of if statements is the job of later posts.

You can follow the whole story in the series Taking over a legacy project. If you're preparing for interviews, questions about clean code and teamwork are in the interview question bank.