Practical Clean Code

BarcelonaJS - 2018-10-22

Israel Saeta Pérez (@dukebody)

TravelPerk

mission:

manage complexity

projects delay/failure

sneaky bugs

developer suffering & quit

contagious mess

What is clean code

  • reads like prose
  • makes it hard for bugs to hide
  • can be enhanced by a developer other than its original author
  • provides one obvious way to make things

1. the prose-programming process

  • Step away from your keyboard
  • Write the logic you want to implement in
    ​a paper
  • Write for non-technical users
  • Use multiple different levels of detail until you get to something you believe you can implement

 

  • Implement each different piece of logic as a separate function

can a person buy beer?

a person can buy beer if it is at least 18 years old

 

  


to compute the age of a person from its birthdate

take the difference from current year and its birthdate year

remove one if its birthday has not yet passed this year

the birthday of a person has passed if the month of its birthday is larger or equal to current month, and the day of its birthday is larger or equal that the current day

can a person buy beer?

function canBuyBeer(person) {
    return person.age >= 18;
}


class Person:
    get age() {
      const birthdate = this.birthdate;
      const now = Date();
      const year_diff = now.getFullYear() - birthdate.getFullYear();
    
      if (this.hasAnniversaryPassed(now, birthdate)) {
        return year_diff;
      } else {
        return year_diff - 1;
      }
} 

function hasAnniversaryPassed(now, anniversary) {
    return now.getMonth() >= anniversary.getMonth() 
            && now.getDate() > anniversary.getDate();
}

2. use meaningful variable and function names

  • Avoid too short names in general (<= 2 chars)
  • Avoid too generic names like stuff, tmp, list, do, process, etc.
  • Take into account the variable/function scope
  • If it is difficult to name, it might be a bad idea
// bad
let ret = [];
for (const l of legs) {
    const s = l.getFirstSegment();
    ret.push(s);
}
return ret;


// better
let firstSegments = []
for (const leg of legs) {
    const segment = leg.getFirstSegment();
    firstSegments.push(segment);
}
return firstSegments;

// bad, too generic variable names
function getPopularity() {
    let popularity = {}

    allPopularity = db.open("popularity_db.csv")
    for (aLine of allPopularity) {
        let tmp = aLine.split("\t")
        if (tmp.length === 2) {
            popularity[tmp[0]] = int(tmp[1].trim())
        }
    }
    return popularity
}


// better
function getPopularityByAirport() {
    let popularities = {}

    popularityDB = db.open("popularity_db.csv")

    for (line of popularityDB) {
        let values = line.split("\t")

        let correctLine = values.length === 2
        if (correctLine) {
            airportCode = values[0]
            airportPopularity = parseInt(values[1].trim())
            popularities[airportCode] = airportPopularity
        }
    }
    return popularities
}

3. break long nested functions into smaller ones

reduce cognitive complexity

detect duplicate trips

Given a trip, its duplicates are those from the same company that have the same signature

// bad
function getDuplicatedTrips(trip) {
    // calculate signature for trip
    let signature = '';
    for (const leg of trip.legs) {
        for (const segment of leg.segments) {
            signature += getSignature(segment);
        }
    }

    // get trips candidate for being duplicate
    const company = trip.company;
    const candidateTrips = db.trips.filter(company=company);

    // calculate signature for each trip
    let candidateSignatures = [];
    for (const candidateTrip of candidateTrips) {
        let candidateSignature = '';
        for (const leg of candidateTrip.legs) {
            for (const segment of leg.segments) {
                candidateSignature += getSignature(segment);
            }
        }
        candidateSignatures.push(candidateSignature);
    }

    // get the trips for which the signature matches
    let duplicatedTrips = [];
    for (let i = 0; i < candidateSignatures.length; i++) {
        let candidateSignature = candidateSingatures[i];
        if (candidatSsignature === signature) {
            duplicatedTrips.push(candidateTrips[i]);
        }
    }

    return duplicatedTrips;
}

DETECT duplicated trips

// better
function getDuplicatedTrips(trip) {
    const signature = getTripSignature(trip);
    const candidateTrips = getCandidateTrips(trip);
    return candidateTrips.filter(trip => getTripSignature(trip) === signature);
}


function getTripSignature(trip) {
    let signature = '';
    for (const leg of trip.legs) {
        for (const segment of leg.segments) {
            signature += getSignature(segment);
        }
    }
    return signature;
}


function getCandidateTrips(trip) {
    const company = trip.company;
    return db.trips.filter(company=company);
}

DETECT duplicated trips

COMPUTE TOTAL INVOICE AMOUNTS FOR MONTH

Get all invoices for the given month and add up their amounts

// bad
function get_total_invoice_amounts(year, month) {
  // get inovices for given month
  const start = Date(year, month, 1);
  const next_month = month != 12 ? month + 1 : 1;
  const end = Date(year, next_month, 1);
  const invoices = db.invoices.filter({
    created_at__gte: start,
    created_at__lte: end
  });

  // sum amounts for invoices
  let total = 0;
  for (const invoice of invoices) {
    for (const item of invoice.items) {
      total += item.amount;
    }
  }
  return total;
}

Compute total invoice amounts for month

// better
function get_total_invoice_amounts(year, month) {
  invoices = get_invoices_for_month(year, month);
  amounts = invoices.map(invoice => get_amount(invoice));
  return sum(amounts);
}

function get_invoices_for_month(year, month) {
  const start = Date(year, month, 1);
  const next_month = month !== 12 ? month + 1 : 1;
  const end = Date(year, next_month, 1);
  return db.invoices.filter({ created_at__gte: start, created_at__lte: end });
}

function get_amount(invoice) {
  // should be internal to class invoice
  const amounts = invoice.items.map(item => item.amount);
  return sum(amounts);
}

function sum(elements) {
  return elements.reduce((element_1, element_2) => element_1 + element_2, 0);
}

Compute total invoice amounts for month

clarify conditionals

// OK
if (['create', 'update', 'delete'].includes(action)) {
    check_permission(user, 'write')
}


// better
is_write_action = ['create', 'update', 'delete'].includes(action)
if (is_write_action) {
    check_permission(user, 'write')
}


// even better
function is_write_action(action) {
    return ['create', 'update', 'delete'].includes(action)
}

if (is_write_action(action)) {
    check_permission(user, 'write')
}

4. hide details safely

with abstraction and encapsulation

abstraction

Manage complexity by separating the (public) object interfaces from their (hidden) implementation details

encapsulation

Bind together data and operations, allowing only operation through certain methods to prevent misuse

Instead of accessing objects attributes directly, create business methods and use only those, transferring defensiveness from the caller to the callee

// bad
// caller needs to know internals
for (const leg of dog.legs) {
    leg.move(10)
}



// better
dog.move(10)

class Dog {
    move(forward) {
        for (const leg of self.legs) {
            leg.move(forward)
        }
    }
}

ask dog move forward

(hide internal structure)

// bad
function getTotalInvoicesAmount(invoices) {
    let total = 0
    for (const invoice of invoices) {
        // exposes internal structure!
        for (const item in invoice.items) {
            total += item.amount
        }
    }
    return total
}


// better
class Invoice {
    get amount() {
        return sum(this.items.map(item => item.amount))
    }
}

function getTotalInvoicesAmount(invoices) {
    let total = 0
    for (const invoice of invoices) {
        total += invoice.amount
    }
    return total
}

hide internal structure

// bad
function pay(product, wallet) {
    // touches internal data!
    wallet.money -= product.price
}


// better
function pay(product, wallet) {
    wallet.take(product.price)
}


class Wallet {
    take(price) {
        if price > this.money:
            throw Error('Not enough money')
        // ...
    }
}

avoid impossible states

(negative money amount)

// risky
const price = {
    'amount': 13,
    'currency': 'EUR'
}


// better
class Price {
     constructor(amount, currency):
        if (this.amount < 0) {
            throw Error('Amount cannot be smaller than zero')
        }
        this.amount = amount

        if (!ALLOWED_CURRENCIES.includes(currency)) {
            throw Error(`Unknown currency ${currency}`)
        }
        this.currency = currency
    }
}


const price = new Price(13, 'EUR')

avoid impossible states

(negative amount, invlaid currency)

6. start simple and do continous refactoring

  • Avoid premature flexibility or features because You Arent Gonna Need It
  • Keep the design short and simple (KISS)
  • Avoid premature abstraction: duplication is far better than the worng abstraction (wait for three examples)
  • Easy to refactor to add features later if needed
  • Scout rule: Always leave code cleaner than you found it

References

thanks! :)

Made with Slides.com