Common Code Smells

 1// ❌ Bad: 200-line method doing everything
 2function processOrder(order) {
 3  // validate order
 4  if (!order.id) throw new Error("Invalid order");
 5  if (!order.items || order.items.length === 0) throw new Error("No items");
 6  
 7  // calculate totals
 8  let subtotal = 0;
 9  for (const item of order.items) {
10    subtotal += item.price * item.quantity;
11  }
12  const tax = subtotal * 0.08;
13  const shipping = subtotal > 100 ? 0 : 10;
14  const total = subtotal + tax + shipping;
15  
16  // update database
17  db.orders.update(order.id, { total, status: 'processing' });
18  
19  // send email
20  emailService.send(order.email, 'Order Confirmation', `Total: $${total}`);
21  
22  // log
23  logger.info(`Order ${order.id} processed`);
24  
25  // ... 180 more lines
26}
27
28// ✅ Good: Broken into smaller, focused functions
29function processOrder(order) {
30  validateOrder(order);
31  const total = calculateTotal(order);
32  updateDatabase(order, total);
33  sendConfirmationEmail(order, total);
34  logOrderProcessed(order);
35}
36
37function validateOrder(order) {
38  if (!order.id) throw new Error("Invalid order");
39  if (!order.items || order.items.length === 0) throw new Error("No items");
40}
41
42function calculateTotal(order) {
43  const subtotal = order.items.reduce((sum, item) => 
44    sum + (item.price * item.quantity), 0);
45  const tax = subtotal * TAX_RATE;
46  const shipping = subtotal > FREE_SHIPPING_THRESHOLD ? 0 : SHIPPING_COST;
47  return subtotal + tax + shipping;
48}

 1// ❌ Bad: 7 parameters
 2function createUser(
 3  name: string,
 4  email: string,
 5  age: number,
 6  address: string,
 7  phone: string,
 8  role: string,
 9  department: string
10) {
11  // ...
12}
13
14// Calling it is error-prone
15createUser("John", "john@example.com", 30, "123 Main St", "555-1234", "admin", "IT");
16
17// ✅ Good: Use object parameter
18interface UserData {
19  name: string;
20  email: string;
21  age: number;
22  address: string;
23  phone: string;
24  role: string;
25  department: string;
26}
27
28function createUser(userData: UserData) {
29  // ...
30}
31
32// Much clearer
33createUser({
34  name: "John",
35  email: "john@example.com",
36  age: 30,
37  address: "123 Main St",
38  phone: "555-1234",
39  role: "admin",
40  department: "IT"
41});

 1// ❌ Bad: One class doing everything
 2class OrderManager {
 3  validateOrder(order) {}
 4  calculateTotal(order) {}
 5  calculateTax(amount) {}
 6  calculateShipping(order) {}
 7  processPayment(order) {}
 8  chargeCard(card, amount) {}
 9  refundPayment(orderId) {}
10  sendEmail(to, subject, body) {}
11  sendSMS(phone, message) {}
12  generateInvoice(order) {}
13  generatePDF(invoice) {}
14  updateInventory(items) {}
15  checkStock(itemId) {}
16  reserveStock(items) {}
17  logTransaction(order) {}
18  logError(error) {}
19  // ... 50 more methods
20}
21
22// ✅ Good: Separate responsibilities
23class OrderValidator {
24  validate(order) {}
25}
26
27class OrderCalculator {
28  calculateTotal(order) {}
29  calculateTax(amount) {}
30  calculateShipping(order) {}
31}
32
33class PaymentProcessor {
34  processPayment(order) {}
35  chargeCard(card, amount) {}
36  refundPayment(orderId) {}
37}
38
39class NotificationService {
40  sendEmail(to, subject, body) {}
41  sendSMS(phone, message) {}
42}
43
44class InvoiceGenerator {
45  generate(order) {}
46  generatePDF(invoice) {}
47}
48
49class InventoryManager {
50  updateInventory(items) {}
51  checkStock(itemId) {}
52  reserveStock(items) {}
53}
54
55class TransactionLogger {
56  logTransaction(order) {}
57  logError(error) {}
58}

 1// ❌ Bad
 2if (user.age > 18 && user.score > 100 && user.accountAge > 365) {
 3  grantPremiumAccess(user);
 4}
 5
 6// What do these numbers mean?
 7const discount = price * 0.15;
 8const shipping = weight > 5 ? 25 : 10;
 9
10// ✅ Good: Named constants
11const LEGAL_AGE = 18;
12const MINIMUM_SCORE = 100;
13const DAYS_IN_YEAR = 365;
14const PREMIUM_DISCOUNT_RATE = 0.15;
15const HEAVY_PACKAGE_THRESHOLD_KG = 5;
16const STANDARD_SHIPPING_COST = 10;
17const HEAVY_SHIPPING_COST = 25;
18
19if (user.age > LEGAL_AGE && 
20    user.score > MINIMUM_SCORE && 
21    user.accountAge > DAYS_IN_YEAR) {
22  grantPremiumAccess(user);
23}
24
25const discount = price * PREMIUM_DISCOUNT_RATE;
26const shipping = weight > HEAVY_PACKAGE_THRESHOLD_KG 
27  ? HEAVY_SHIPPING_COST 
28  : STANDARD_SHIPPING_COST;

 1// ❌ Bad: Arrow of doom
 2if (user) {
 3  if (user.isActive) {
 4    if (user.hasPermission('write')) {
 5      if (user.balance > 0) {
 6        if (!user.isSuspended) {
 7          // do something
 8          return result;
 9        }
10      }
11    }
12  }
13}
14
15// ✅ Good: Early returns (guard clauses)
16if (!user) return;
17if (!user.isActive) return;
18if (!user.hasPermission('write')) return;
19if (user.balance <= 0) return;
20if (user.isSuspended) return;
21
22// do something
23return result;
24
25// ✅ Also good: Extract to function
26function canUserWrite(user) {
27  return user &&
28         user.isActive &&
29         user.hasPermission('write') &&
30         user.balance > 0 &&
31         !user.isSuspended;
32}
33
34if (canUserWrite(user)) {
35  // do something
36  return result;
37}

 1// ❌ Bad
 2function processData(data) {
 3  // const oldWay = data.map(x => x * 2);
 4  // return oldWay.filter(x => x > 10);
 5  
 6  // const anotherWay = data.filter(x => x > 5).map(x => x * 2);
 7  
 8  return data.map(x => x * 2).filter(x => x > 10);
 9}
10
11// ✅ Good: Remove it (it's in git history if you need it)
12function processData(data) {
13  return data.map(x => x * 2).filter(x => x > 10);
14}

 1// ❌ Bad: Inconsistent naming
 2const usr = getUser();
 3const userInfo = getUserData();
 4const u = fetchUserDetails();
 5const user_profile = loadUserProfile();
 6
 7function get_user_by_id(id) {}
 8function fetchUserByEmail(email) {}
 9function UserByName(name) {}
10
11// ✅ Good: Consistent naming
12const user = getUser();
13const userDetails = getUserDetails();
14const userProfile = getUserProfile();
15const userSettings = getUserSettings();
16
17function getUserById(id) {}
18function getUserByEmail(email) {}
19function getUserByName(name) {}

 1// ❌ Bad: What does true mean here?
 2sendEmail(user, true);
 3processOrder(order, false, true);
 4createUser(data, true, false, true);
 5
 6// ✅ Good: Use options object or separate functions
 7interface EmailOptions {
 8  urgent: boolean;
 9}
10
11sendEmail(user, { urgent: true });
12
13// Or use separate functions
14sendUrgentEmail(user);
15sendRegularEmail(user);
16
17// Or use enums
18enum Priority {
19  URGENT,
20  NORMAL
21}
22
23sendEmail(user, Priority.URGENT);

 1// ❌ Bad: Primitives everywhere
 2function createUser(
 3  email: string,
 4  phone: string,
 5  zipCode: string
 6) {
 7  // Easy to mix up parameters
 8  // No validation
 9}
10
11createUser("555-1234", "john@example.com", "12345"); // Oops, wrong order!
12
13// ✅ Good: Use value objects
14class Email {
15  constructor(private value: string) {
16    if (!this.isValid(value)) {
17      throw new Error("Invalid email");
18    }
19  }
20  
21  private isValid(email: string): boolean {
22    return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email);
23  }
24  
25  toString(): string {
26    return this.value;
27  }
28}
29
30class PhoneNumber {
31  constructor(private value: string) {
32    if (!this.isValid(value)) {
33      throw new Error("Invalid phone number");
34    }
35  }
36  
37  private isValid(phone: string): boolean {
38    return /^\d{3}-\d{3}-\d{4}$/.test(phone);
39  }
40  
41  toString(): string {
42    return this.value;
43  }
44}
45
46function createUser(
47  email: Email,
48  phone: PhoneNumber,
49  zipCode: ZipCode
50) {
51  // Type safe, validated, clear intent
52}
53
54// Can't mix up the order - compiler error!
55createUser(
56  new Email("john@example.com"),
57  new PhoneNumber("555-1234"),
58  new ZipCode("12345")
59);

 1// ❌ Bad
 2class User {
 3  constructor(
 4    name: string,
 5    email: string,
 6    age: number,
 7    address: string,
 8    phone: string,
 9    role: string,
10    department: string,
11    manager: string
12  ) {
13    // ...
14  }
15}
16
17// ✅ Good: Use builder pattern
18class UserBuilder {
19  private name: string;
20  private email: string;
21  private age: number;
22  private address: string;
23  private phone: string;
24  private role: string;
25  private department: string;
26  private manager: string;
27  
28  setName(name: string): this {
29    this.name = name;
30    return this;
31  }
32  
33  setEmail(email: string): this {
34    this.email = email;
35    return this;
36  }
37  
38  // ... other setters
39  
40  build(): User {
41    return new User(this);
42  }
43}
44
45const user = new UserBuilder()
46  .setName("John")
47  .setEmail("john@example.com")
48  .setAge(30)
49  .build();

 1// ❌ Bad: OrderProcessor is envious of Order's data
 2class OrderProcessor {
 3  calculateTotal(order: Order) {
 4    let total = 0;
 5    for (const item of order.items) {
 6      total += item.price * item.quantity;
 7    }
 8    const tax = total * order.taxRate;
 9    const shipping = order.shippingCost;
10    return total + tax + shipping;
11  }
12}
13
14// ✅ Good: Move the method to where the data is
15class Order {
16  items: Item[];
17  taxRate: number;
18  shippingCost: number;
19  
20  calculateTotal(): number {
21    const subtotal = this.items.reduce(
22      (sum, item) => sum + (item.price * item.quantity), 
23      0
24    );
25    const tax = subtotal * this.taxRate;
26    return subtotal + tax + this.shippingCost;
27  }
28}