Code smells: long method, large class, duplication, primitives instead of types, long if chains

Code in:JavaC#PythonJavaScript

In short: A code smell is a sign that the next change will breed a bug: a long method, a large class, duplication, primitives instead of types and a long chain of ifs.

Problem

The checkout says €90, the invoice says €95

The order total is computed in two places: at the checkout and in the invoice. A month ago the VIP discount became 10%. The developer fixed the checkout:

function checkoutTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.1 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

function invoiceTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.05 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

console.log(`Checkout: €${checkoutTotal(100, true).toFixed(2)}`);
console.log(`Invoice: €${invoiceTotal(100, true).toFixed(2)}`);

Output

Checkout: €90.00
Invoice: €95.00

The customer pays €90, and the invoice says €95. Accounting finds a €5 difference in every VIP order for the past month. No line here is "wrong" by itself: invoiceTotal does exactly what it was written to do. The problem is that one rule lives in two places.

See it

How two copies drift apart

function checkoutTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.1 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

function invoiceTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.05 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

console.log(`Checkout: €${checkoutTotal(100, true).toFixed(2)}`);
console.log(`Invoice: €${invoiceTotal(100, true).toFixed(2)}`);

Step by step, in words

1. invoiceTotal started as a copy-paste of checkoutTotal: the same three lines, both with a 5% VIP discount.
2. The VIP discount becomes 10%. The bug report came from the checkout, so the fix goes into checkoutTotal, line 2.
3. Nobody remembers that the invoice is a copy. Line 8 keeps 0.05: the two copies now say different things.
4. A €100 order of a VIP customer goes through the checkout: 100 − 10 + 0 = €90.
5. The same order goes into the invoice: 100 − 5 + 0 = €95.
6. The customer pays €90, and the invoice says €95. One rule in two places: you fix one, the other stays behind.

On the day of the copy-paste both functions were right. The bug was born later, with a fix that was made honestly, but in one place. Code that is right today but will cause a bug with the next change has a code smell. The term comes from Kent Beck and Martin Fowler: a smell is not an error, but a sign that something near it will soon go wrong.

This lesson is about the five most common smells and how to notice them before they cost you €5 an order.

The idea

Duplication: one rule, one place

The rule for the total becomes one function, and the checkout and the invoice both call it:

function orderTotal(subtotal, vip) {
  const discount = vip ? subtotal * 0.1 : 0;
  const delivery = subtotal >= 50 ? 0 : 4.9;
  return subtotal - discount + delivery;
}

console.log(`Checkout: €${orderTotal(100, true).toFixed(2)}`);
console.log(`Invoice: €${orderTotal(100, true).toFixed(2)}`);

Output

Checkout: €90.00
Invoice: €90.00

Now you cannot fix one and forget the other: there is no other.

How you smell it. You fix a bug and ask yourself: "Was this copied somewhere?". You search for a distinctive number or expression (0.05) and find it in two places. Two functions with almost the same names (checkout…, invoice…) and almost the same bodies.

Careful. Not everything that looks the same is duplication. Free delivery from €50 and returns within 50 days are two rules with one number. Merge them into one constant, and changing one changes the other. Duplication is one rule in two places. Many teams wait for the third copy before they pull out the common part: two can be a coincidence, three rarely are.

See it

Long method

The function takes an order and does everything:

function placeOrder(email, subtotal, city) {
  // check
  if (!email.includes("@")) {
    console.log("Invalid email: " + email);
    return;
  }
  if (subtotal <= 0) {
    console.log("Empty cart");
    return;
  }
  // total
  let delivery = city === "Sofia" ? 3.9 : 5.9;
  if (subtotal >= 50) {
    delivery = 0;
  }
  const total = subtotal + delivery;
  // notification
  const text = `Your order for €${total.toFixed(2)} is accepted.`;
  console.log("To " + email + ": " + text);
  // log
  console.log(`[log] order ${email} ${total.toFixed(2)}`);
}

placeOrder("ana@example.com", 34, "Plovdiv");
placeOrder("ana.example.com", 34, "Plovdiv");

Output

To ana@example.com: Your order for €39.90 is accepted.
[log] order ana@example.com 39.90
Invalid email: ana.example.com

How you smell it: you scroll to see the end; comments work as headings for parts ("check", "total"); you cannot say what it does without saying "and"; a variable declared at the top is used only at the bottom. Every heading comment is the name of a function waiting to happen (see the lesson Comments that lie).

Each part goes into its own function, named after its comment:

function problemWith(email, subtotal) {
  if (!email.includes("@")) {
    return "Invalid email: " + email;
  }
  if (subtotal <= 0) {
    return "Empty cart";
  }
  return null;
}

function deliveryFee(subtotal, city) {
  if (subtotal >= 50) {
    return 0;
  }
  return city === "Sofia" ? 3.9 : 5.9;
}

function notifyCustomer(email, total) {
  const text = `Your order for €${total.toFixed(2)} is accepted.`;
  console.log("To " + email + ": " + text);
}

function log(email, total) {
  console.log(`[log] order ${email} ${total.toFixed(2)}`);
}

function placeOrder(email, subtotal, city) {
  const problem = problemWith(email, subtotal);
  if (problem !== null) {
    console.log(problem);
    return;
  }
  const total = subtotal + deliveryFee(subtotal, city);
  notifyCustomer(email, total);
  log(email, total);
}

placeOrder("ana@example.com", 34, "Plovdiv");
placeOrder("ana.example.com", 34, "Plovdiv");

Output

To ana@example.com: Your order for €39.90 is accepted.
[log] order ana@example.com 39.90
Invalid email: ana.example.com

The output is the same, letter for letter. The main function now reads like a list of steps, and the delivery fee can be tested on its own. This step is called Extract Method, and you will do it in small steps in the lesson on refactoring.

See it

Large class

The order has collected everything anyone ever needed:

class Order {
  customerName = "";
  customerEmail = "";
  customerPhone = "";
  deliveryCity = "";
  deliveryStreet = "";
  deliveryPostcode = "";
  cardNumber = "";
  cardExpiry = "";
  subtotal = 0;
  discount = 0;
  smsSent = false;
  emailSent = false;
}

const order = new Order();
order.deliveryCity = "Varna";
console.log("Delivery to " + order.deliveryCity);

Output

Delivery to Varna

How you smell it: groups of fields with a common prefix (customer…, delivery…, card…); methods that use only one group; a name like OrderManager or Utils that says nothing; every task on the team touches this file, and the git merge conflicts are always in it.

Every group with a common prefix is a type waiting for a name:

class Order {
  customer = null;
  delivery = null;
  card = null;
  subtotal = 0;
  discount = 0;
  smsSent = false;
  emailSent = false;
}

const order = new Order();
order.delivery = { city: "Varna", street: "5 Morska St", postcode: "9000" };
console.log("Delivery to " + order.delivery.city);

Output

Delivery to Varna

In JavaScript a group is often a plain object { city, street, postcode }. A class is worth it when the group has its own rules, like the phone below.

The rest (smsSent, emailSent) is the next group. You do not have to fix everything at once.

Try it

Predict the output

Three products at €0.10 each. What does the program print?

let total = 0;
for (let i = 0; i < 3; i++) {
  total += 0.1;
}
console.log(total === 0.3);
console.log(total);

Answer

false / 0.30000000000000004

true / 0.30

true / 0.3

Check

0.10 has no exact binary form, just as 1/3 has no exact decimal form. A double keeps the nearest binary number, and the small errors add up.

See it

Primitives instead of types: money

Money in a double is the most common form of the smell primitive obsession: a value with its own rules lives in a type that does not know those rules. So far in the course the amounts were number to keep things short. In real code:

let cents = 0;
for (let i = 0; i < 3; i++) {
  cents += 10;
}
console.log(cents === 30);
console.log((cents / 100).toFixed(2));

Output

true
0.30

In JavaScript every number is a double, and there is no decimal type. So money is kept in whole cents: 10 + 10 + 10 is exactly 30. It becomes euros only when you print it.

let cents = 0;
for (let i = 0; i < 3; i++) {
  cents += ;
}
console.log(cents === 30);
console.log((cents / 100).toFixed(2));

Check

More precisely, if you are curious

Better still is a Money type that keeps the amount and the currency together and does not let you add euros to dollars by mistake. It is the same idea as the phone on the next card.

See it

Primitives instead of types: the phone

A phone number as a string is another form of the same smell. "0888 123 456", "+359 888 123 456" and "+359888123456" are one number, but three different strings. Every place that compares phones has to remember on its own how to clean them. The Phone type cleans and checks once, when it is created:

class Phone {
  #value;

  constructor(raw) {
    let digits = raw.replace(/[ ()-]/g, "");
    if (digits.startsWith("0")) {
      digits = "+359" + digits.slice(1);
    }
    if (!/^\+359\d{9}$/.test(digits)) {
      throw new Error("Invalid phone: " + raw);
    }
    this.#value = digits;
  }

  get value() {
    return this.#value;
  }

  equals(other) {
    return other instanceof Phone && other.#value === this.#value;
  }
}

const a = new Phone("0888 123 456");
const b = new Phone("+359 888 123 456");
console.log(a.value);
console.log(a.equals(b) ? "same number" : "different numbers");
try {
  new Phone("0888 12");
} catch (e) {
  console.log(e.message);
}

Output

+359888123456
same number
Invalid phone: 0888 12

The regular expression [ ()-] removes spaces, brackets and dashes, and \+359\d{9} checks that +359 is followed by exactly nine digits.

An invalid phone cannot exist: the constructor does not let it through. Every Phone in the program is already clean.

In JavaScript === between two objects checks whether they are the same object, not whether they are equal. That is why Phone has its own equals, and #value is a private field that nobody outside can change.

Put the lines of the constructor in order so that "0888 123 456" becomes "+359888123456":

1. digits = "+359" + digits.slice(1);↑↓
2. }↑↓
3. }↑↓
4. this.#value = digits;↑↓
5. let digits = raw.replace(/[ ()-]/g, "");↑↓
6. if (!/^\+359\d{9}$/.test(digits)) {↑↓
7. throw new Error("Invalid phone: " + raw);↑↓
8. if (digits.startsWith("0")) {↑↓

Check

Try it

Find the bug

The customer has unsubscribed from SMS messages, and the shop still texts them about every parcel:

const optedOut = ["+359888123456"];
notifyShipped("0888 123 456", optedOut);
notifyShipped("0877 555 010", optedOut);

Output

SMS to 0888 123 456: Your order is on its way.
SMS to 0877 555 010: Your order is on its way.

Which line has the bug?

Pick the line with the bug.

1function notifyShipped(phone, optedOut) {2 if (optedOut.includes(phone)) {3 return;4 }5 console.log("SMS to " + phone + ": Your order is on its way.");6}

Check

See it

Long if chain on a type

The price and the delivery time depend on the delivery type. The chain is in both functions:

function price(kind) {
  if (kind === "courier") {
    return 5.9;
  } else if (kind === "office") {
    return 3.9;
  } else if (kind === "locker") {
    return 2.9;
  }
  throw new Error("Unknown delivery type: " + kind);
}

function days(kind) {
  if (kind === "courier") {
    return 1;
  } else if (kind === "office") {
    return 2;
  } else if (kind === "locker") {
    return 3;
  }
  throw new Error("Unknown delivery type: " + kind);
}

for (const kind of ["courier", "office", "locker"]) {
  console.log(`${kind}: €${price(kind).toFixed(2)}, days: ${days(kind)}`);
}

Output

courier: €5.90, days: 1
office: €3.90, days: 2
locker: €2.90, days: 3

How you smell it: the same chain on the same field (kind, type, status) in several places; a new type means finding every chain; a forgotten branch shows up only when a customer picks it.

Here the types differ only by numbers. Then the chain becomes a table: type → data:

const DELIVERY = new Map([
  ["courier", { price: 5.9, days: 1 }],
  ["office", { price: 3.9, days: 2 }],
  ["locker", { price: 2.9, days: 3 }],
]);

for (const kind of ["courier", "office", "locker"]) {
  const d = DELIVERY.get(kind);
  console.log(`${kind}: €${d.price.toFixed(2)}, days: ${d.days}`);
}

Output

courier: €5.90, days: 1
office: €3.90, days: 2
locker: €2.90, days: 3

A new type is a new row in the table. When the types differ by behavior, not just by numbers, each type becomes its own class or function: that is Strategy. If the type changes over time (the order is new, then paid, then shipped), that is State. How a chain turns into classes step by step (Replace Conditional with Polymorphism) is in the lesson on refactoring.

The idea

A smell is not a bug: when to leave it

The five smells in short:

• duplication: one rule in several places → one function or constant;
• long method: you scroll and read heading comments → Extract Method;
• large class: groups of fields with a common prefix → a class for each group;
• primitives instead of types: money in a double, a phone in a string → a type that keeps its own rules;
• long if chain on a type: the same chain in many places → a table, Strategy or State.

A smell is not a bug. It tells you what the next change will cost. So you leave it when:

• the code does not change. The report nobody has touched in three years costs nothing until you touch it;
• there are no tests. Without them, fixing the smell can break something that worked. Tests first (the next lesson), then small steps;
• two things only look the same. Two rules with one number are not duplication;
• the code is for one use: a migration script you will delete tomorrow.

The scout rule: when you are touching a file anyway, leave it a bit cleaner than you found it. Do not rewrite whole modules "just in case".

Exercise

Smell it and fix it

Solve it in your own editor.

The receipt. The function sends a receipt by SMS, email or Viber:

function sendReceipt(channel, contact, amount) {
  if (channel === "sms") {
    const text = `You paid €${amount.toFixed(2)}. Thank you!`;
    console.log("SMS to " + contact.replaceAll(" ", "") + ": " + text);
  } else if (channel === "email") {
    const text = `You paid €${amount.toFixed(2)}. Thank you!`;
    console.log("Email to " + contact.trim().toLowerCase() + ": " + text);
  } else if (channel === "viber") {
    const text = `You paid €${amount.toFixed(2)}. Thank you!`;
    console.log("Viber to " + contact.replaceAll(" ", "") + ": " + text);
  }
}

sendReceipt("sms", "0888 123 456", 24.5);
sendReceipt("email", " Ana@Example.com ", 24.5);
sendReceipt("viber", "0877 555 010", 9.9);

Output

SMS to 0888123456: You paid €24.50. Thank you!
Email to ana@example.com: You paid €24.50. Thank you!
Viber to 0877555010: You paid €9.90. Thank you!

Find the smells in it (at least three) and name them. Then fix them one at a time, running the program after each step: the output must stay the same, letter for letter.

More tasks

• A new channel. Add Telegram, with a contact like the SMS one. In how many places do you make changes before the fix, and in how many after it?
• Money in cents. Change the receipt amount to whole cents (2450), and turn it into euros only when printing.
• The large class. Finish Order from the card "Large class": move smsSent and emailSent into their own type and think about what to call it.

Test

Check yourself

1. The code has a smell. Does that mean it has a bug?

Yes, a smell is a bug by another nameYes, if the compiler gives a warningNo. The code may work today; the smell says the next change is risky

2. Free delivery starts at €50, and returns are allowed for up to 50 days. Should you make them one constant to avoid duplication?

No. These are two different rules that happen to have the same numberYes, one number should live in one placeYes, but call it FIFTY

3. Totals add up in a double, and 0.10 three times is not 0.30. What helps?

A bigger type: float instead of doubleRounding every time you printWhole cents instead of euros

4. The same if chain on the delivery type is in the price, in the delivery time and in the label. A new type arrives. What helps?

A comment above each chain listing the typesOne more else if in all three placesA table type → data, or a class per type if the behavior differs

5. The yearly report module: 300 lines in one method, nobody has touched it in three years, no tests. Tomorrow you have to ship something else. What do you do?

Split it into functions todayDelete itLeave it. You will fix it when you need to change it, and you will write tests for it first

Check

Start the lesson over

Related lessons

• Builds on: State · Names in code · Functions: one job, few parameters, no hidden side effects · Comments that lie and comments that help
• Continues in: Tests as a safety net · Refactoring in small steps · SOLID without dogma · Exercise: a messy class, cleaned up step by step
• Tasks on this topic: Clean code and refactoring