Clean Code

// Which would you rather debug at 2am?

// Version A
function p(a, b) {
  let x = 0
  for (let i = 0; i < a.length; i++) {
    if (a[i].s === 1) x += a[i].p * b
  }
  return x
}

// Version B
function calculateActiveProductsTotal(products, taxRate) {
  let total = 0
  for (const product of products) {
    if (product.status === PRODUCT_STATUS.ACTIVE) {
      total += product.price * taxRate
    }
  }
  return total
}

┌─────────────────────────────────────────────────────────────────────────┐
│                     CODE LIKE A NEWSPAPER                                │
├─────────────────────────────────────────────────────────────────────────┤
│                                                                          │
│   // HEADLINE: What does this module do?                                 │
│   export function processUserOrder(userId, orderId) {                    │
│     const user = getUser(userId)                                         │
│     const order = getOrder(orderId)                                      │
│     validateOrder(user, order)                                           │
│     return chargeAndShip(user, order)                                    │
│   }                                                                      │
│                                                                          │
│   // DETAILS: How does it do it?                                         │
│   function getUser(userId) { ... }                                       │
│   function getOrder(orderId) { ... }                                     │
│   function validateOrder(user, order) { ... }                            │
│   function chargeAndShip(user, order) { ... }                            │
│                                                                          │
│   Read top-to-bottom. The "what" comes before the "how".                 │
│                                                                          │
└─────────────────────────────────────────────────────────────────────────┘

// ❌ What does this even mean?
const yyyymmdstr = moment().format('YYYY/MM/DD')
const d = new Date()
const t = d.getTime()

// ✓ Crystal clear
const currentDate = moment().format('YYYY/MM/DD')
const now = new Date()
const timestamp = now.getTime()

// ❌ Inconsistent - which one do I use?
getUserInfo()
fetchClientData()
retrieveCustomerRecord()

// ✓ Consistent vocabulary
getUser()
getClient()
getCustomer()

// ❌ What is 'l'? A number? A location? A letter?
locations.forEach(l => {
  doStuff()
  // ... 50 lines later
  dispatch(l)  // Wait, what was 'l' again?
})

// ✓ No guessing required
locations.forEach(location => {
  doStuff()
  dispatch(location)
})

// ❌ Redundant prefixes
const Car = {
  carMake: 'Honda',
  carModel: 'Accord',
  carColor: 'Blue'
}

// ✓ Context is already clear
const Car = {
  make: 'Honda',
  model: 'Accord',
  color: 'Blue'
}

// ❌ This function does too many things
function emailClients(clients) {
  clients.forEach(client => {
    const clientRecord = database.lookup(client)
    if (clientRecord.isActive()) {
      email(client)
    }
  })
}

// ✓ Each function has one job
function emailActiveClients(clients) {
  clients
    .filter(isActiveClient)
    .forEach(email)
}

function isActiveClient(client) {
  const clientRecord = database.lookup(client)
  return clientRecord.isActive()
}

// ❌ What do these arguments mean?
createMenu('Settings', 'User preferences', 'Save', true)

// ✓ Self-documenting with destructuring
createMenu({
  title: 'Settings',
  body: 'User preferences',
  buttonText: 'Save',
  cancellable: true
})

function createMenu({ title, body, buttonText, cancellable = false }) {
  // ...
}

// ❌ Boolean flag = function does two things
function createFile(name, isTemp) {
  if (isTemp) {
    fs.create(`./temp/${name}`)
  } else {
    fs.create(name)
  }
}

// ✓ Two focused functions
function createFile(name) {
  fs.create(name)
}

function createTempFile(name) {
  createFile(`./temp/${name}`)
}

// ❌ What is 86400000? Why 18?
setTimeout(blastOff, 86400000)

if (user.age > 18) {
  allowAccess()
}

if (status === 1) {
  // ...
}

// ✓ Named constants are searchable and self-documenting
const MILLISECONDS_PER_DAY = 60 * 60 * 24 * 1000
const MINIMUM_LEGAL_AGE = 18
const STATUS = {
  ACTIVE: 1,
  INACTIVE: 0
}

setTimeout(blastOff, MILLISECONDS_PER_DAY)

if (user.age > MINIMUM_LEGAL_AGE) {
  allowAccess()
}

if (status === STATUS.ACTIVE) {
  // ...
}

// ❌ Duplicate logic
function showDeveloperList(developers) {
  developers.forEach(dev => {
    const salary = dev.calculateSalary()
    const experience = dev.getExperience()
    const githubLink = dev.getGithubLink()
    render({ salary, experience, githubLink })
  })
}

function showManagerList(managers) {
  managers.forEach(mgr => {
    const salary = mgr.calculateSalary()
    const experience = mgr.getExperience()
    const portfolio = mgr.getPortfolio()
    render({ salary, experience, portfolio })
  })
}

// ✓ Unified with type-specific handling
function showEmployeeList(employees) {
  employees.forEach(employee => {
    const baseData = {
      salary: employee.calculateSalary(),
      experience: employee.getExperience()
    }
    
    const extraData = employee.type === 'developer'
      ? { githubLink: employee.getGithubLink() }
      : { portfolio: employee.getPortfolio() }
    
    render({ ...baseData, ...extraData })
  })
}

// ❌ Mutates the original array - side effect!
function addItemToCart(cart, item) {
  cart.push({ item, date: Date.now() })
}

// ✓ Returns a new array - no side effects
function addItemToCart(cart, item) {
  return [...cart, { item, date: Date.now() }]
}

// ❌ Modifies global state
let name = 'Ryan McDermott'

function splitName() {
  name = name.split(' ')  // Mutates global!
}

// ✓ Pure function - no globals modified
function splitName(name) {
  return name.split(' ')
}

const fullName = 'Ryan McDermott'
const nameParts = splitName(fullName)

// ❌ Deeply nested - hard to follow
function getPayAmount(employee) {
  let result
  if (employee.isSeparated) {
    result = { amount: 0, reason: 'separated' }
  } else {
    if (employee.isRetired) {
      result = { amount: 0, reason: 'retired' }
    } else {
      // ... complex salary calculation
      result = { amount: salary, reason: 'employed' }
    }
  }
  return result
}

// ✓ Guard clauses - flat and readable
function getPayAmount(employee) {
  if (employee.isSeparated) {
    return { amount: 0, reason: 'separated' }
  }
  
  if (employee.isRetired) {
    return { amount: 0, reason: 'retired' }
  }
  
  // Main logic at the top level
  const salary = calculateSalary(employee)
  return { amount: salary, reason: 'employed' }
}

// ❌ Unnecessary nesting
for (const user of users) {
  if (user.isActive) {
    if (user.hasPermission) {
      processUser(user)
    }
  }
}

// ✓ Flat and scannable
for (const user of users) {
  if (!user.isActive) continue
  if (!user.hasPermission) continue
  
  processUser(user)
}

// ❌ These comments add nothing
function hashIt(data) {
  // The hash
  let hash = 0
  
  // Length of string
  const length = data.length
  
  // Loop through every character
  for (let i = 0; i < length; i++) {
    // Get character code
    const char = data.charCodeAt(i)
    // Make the hash
    hash = (hash << 5) - hash + char
    // Convert to 32-bit integer
    hash &= hash
  }
  return hash
}

// ✓ Only comment what's not obvious
function hashIt(data) {
  let hash = 0
  const length = data.length
  
  for (let i = 0; i < length; i++) {
    const char = data.charCodeAt(i)
    hash = (hash << 5) - hash + char
    hash &= hash  // Convert to 32-bit integer
  }
  return hash
}

// ❌ Dead code cluttering the file
doStuff()
// doOtherStuff()
// doSomeMoreStuff()
// doSoMuchStuff()

// ✓ Clean
doStuff()

// ❌ This is what git history is for
/**
 * 2016-12-20: Removed monads (RM)
 * 2016-10-01: Added special monads (JP)
 * 2016-02-03: Removed type-checking (LI)
 */
function combine(a, b) {
  return a + b
}

// ✓ Just the code
function combine(a, b) {
  return a + b
}

```javascript
// ❌ UserSettings handles both settings AND authentication
class UserSettings {
  constructor(user) {
    this.user = user
  }
  
  changeSettings(settings) {
    if (this.verifyCredentials()) {
      // update settings
    }
  }
  
  verifyCredentials() {
    // authentication logic
  }
}

// ✓ Separate responsibilities
class UserAuth {
  constructor(user) {
    this.user = user
  }
  
  verifyCredentials() {
    // authentication logic
  }
}

class UserSettings {
  constructor(user, auth) {
    this.user = user
    this.auth = auth
  }
  
  changeSettings(settings) {
    if (this.auth.verifyCredentials()) {
      // update settings
    }
  }
}
```

```javascript
// ❌ Must modify this function for every new shape
function getArea(shape) {
  if (shape.type === 'circle') {
    return Math.PI * shape.radius ** 2
  } else if (shape.type === 'rectangle') {
    return shape.width * shape.height
  }
  // Add another if for every new shape...
}

// ✓ Extend by adding new classes
class Shape {
  getArea() {
    throw new Error('getArea must be implemented')
  }
}

class Circle extends Shape {
  constructor(radius) {
    super()
    this.radius = radius
  }
  
  getArea() {
    return Math.PI * this.radius ** 2
  }
}

class Rectangle extends Shape {
  constructor(width, height) {
    super()
    this.width = width
    this.height = height
  }
  
  getArea() {
    return this.width * this.height
  }
}
```

```javascript
// ❌ Square breaks when used where Rectangle is expected
class Rectangle {
  constructor() {
    this.width = 0
    this.height = 0
  }
  
  setWidth(width) {
    this.width = width
  }
  
  setHeight(height) {
    this.height = height
  }
  
  getArea() {
    return this.width * this.height
  }
}

class Square extends Rectangle {
  setWidth(width) {
    this.width = width
    this.height = width  // Breaks LSP!
  }
  
  setHeight(height) {
    this.width = height
    this.height = height
  }
}

// This fails for Square - expects 20, gets 25
function calculateAreas(rectangles) {
  rectangles.forEach(rect => {
    rect.setWidth(4)
    rect.setHeight(5)
    console.log(rect.getArea())  // Square returns 25, not 20!
  })
}

// ✓ Better: separate classes, no inheritance relationship
class Rectangle {
  constructor(width, height) {
    this.width = width
    this.height = height
  }
  
  getArea() {
    return this.width * this.height
  }
}

class Square {
  constructor(side) {
    this.side = side
  }
  
  getArea() {
    return this.side * this.side
  }
}
```

```javascript
// ❌ Forcing clients to provide options they don't need
class DOMTraverser {
  constructor(settings) {
    this.settings = settings
    this.rootNode = settings.rootNode
    this.settings.animationModule.setup()  // Required even if not needed!
  }
}

const traverser = new DOMTraverser({
  rootNode: document.body,
  animationModule: { setup() {} }  // Must provide even if not animating
})

// ✓ Make features optional
class DOMTraverser {
  constructor(settings) {
    this.settings = settings
    this.rootNode = settings.rootNode
    
    if (settings.animationModule) {
      settings.animationModule.setup()
    }
  }
}

const traverser = new DOMTraverser({
  rootNode: document.body
  // animationModule is optional now
})
```

```javascript
// ❌ Tightly coupled to InventoryRequester
class InventoryTracker {
  constructor(items) {
    this.items = items
    this.requester = new InventoryRequester()  // Hard dependency
  }
}

// ✓ Dependency injection
class InventoryTracker {
  constructor(items, requester) {
    this.items = items
    this.requester = requester  // Injected - can be any requester
  }
}
```

**Answer:**

The name `process` is too vague. It doesn't tell you what kind of processing happens or what kind of data is expected. Better names would be `validateUserInput`, `parseJsonResponse`, or `calculateOrderTotal`, depending on what the function actually does.

**Answer:**

Too many parameters (5). It's hard to remember the order, and the boolean flags (`isAdmin`, `sendWelcomeEmail`) suggest the function might be doing multiple things. Refactor to use an options object:

```javascript
function createUser({ name, email, age, isAdmin = false }) {
  // ...
}

function sendWelcomeEmail(user) {
  // Separate function for separate concern
}
```

Write comments when you need to explain *why* something is done a certain way, especially for:
- Business logic that isn't obvious from the code
- Workarounds for bugs or edge cases
- Legal or licensing requirements
- Complex algorithms where the approach isn't self-evident

Don't write comments that explain *what* the code does. If the code needs explanation, rewrite it to be clearer.

A magic number is an unexplained numeric literal in code, like `86400000` or `18`. They're bad because:
- You can't search for what they mean
- They don't explain their purpose
- If the value needs to change, you have to find every occurrence

Replace with named constants: `MILLISECONDS_PER_DAY` or `MINIMUM_LEGAL_AGE`.

**Answer:**

Use guard clauses (early returns) to flatten the nesting:

```javascript
function processUser(user) {
  if (!user) return null
  if (!user.isActive) return null
  if (!user.hasPermission) return null
  
  return doSomething(user)
}
```

Each guard clause handles one edge case, and the main logic sits at the top level without indentation.