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
}