Live code review
at LesFurets.com

Alexandre DuBreuil

Jeremy Maltis

Alexandre DuBreuil

Tech lead team insurer panel

Jeremy Maltis

Front developer team journey

Code review is a process...

Our code review process is based on a git flow branching strategy and does not require any tool.

How do you ask for a code review?

You request a code review by changing the status in JIRA to code review (lesfurets.com), by doing a pull request (github) or a merge request (gitlab).

Code review topics

(1) Style and naming

(2) Language and API usage

(3) Simplicity

(4) Object oriented programming

Example 1

Code style and naming


void check(MainDriver d1, Driver d2, Besoins b) 
                        throws ExclusionException {

  int age = DateHelper.getAge(driver.getBirthDate(),
  DateHelper.getToday(),
  EPrecision.day);

  if (age < 18)
      throw new ExclusionException("Must be at least 18 years old");
}
        

// Use precise method naming, parameter naming,
// and remove unused parameters
void check(MainDriver d1, Driver d2, Requirements r) 
                        throws ExclusionException {
  // Could use static imports if clearer and correct indentation
  int age = DateHelper.getAge(driver.getBirthDate(),
  DateHelper.getToday(),
  EPrecision.day);
  // Maybe use constants for magic numbers and string
  if (age < 18)
      // Use brackets for single line statements
      throw new ExclusionException("Must be at least 18 years old");
}
        

void checkAge(MainDriver mainDriver) throws ExclusionException {
  int age = getAge(driver.getBirthDate(), getToday(), EPrecision.day);
  if (age < MINIMUM_AGE) {
      throw new ExclusionException("Must be at least 18 years old");
  }
}

Style and naming wrap up

  • Code review is a discussion and you can discuss about which elements to correct
  • Define a style guide and naming rules with your team as a basis for discussion
  • Example: Google C++ guide
  • Most of it can be automated:
  •     IDEs: use automatic formatting
  •     Sonar: static code analysis

Example 2

Simplicity


private boolean isPreviousVersion(String version, String compareWith) {
    String[] versionDigits = compare.split("\\.");
    String[] compareDigits = compareWith.split("\\.");
    if (valueOf(versionDigits[0]) < valueOf(compareDigits[0]))
        return true;
    else if (valueOf(versionDigits[0]) == valueOf(compareDigits[0])) {
        if (valueOf(versionDigits[1]) < valueOf(compareDigits[1]))
            return true;
        else {
          if (valueOf(versionDigits[1]) == valueOf(compareDigits[1]))
              return valueOf(versionDigits[2]) < valueOf(compareDigits[2]);
          return false;
        }
    }
    return false;
}
        

// General remark: you only compare the first 3 numbers?
private boolean isPreviousVersion(String version, String compareWith) {
    String[] versionDigits = compare.split("\\.");
    String[] compareDigits = compareWith.split("\\.");
    if (valueOf(versionDigits[0]) < valueOf(compareDigits[0]))
        return true;
    // Do not compare Integer with == (it only works up to 127)
    // either use int instead, or use .equals if Integer
    else if (valueOf(versionDigits[0]) == valueOf(compareDigits[0])) {
        if (valueOf(versionDigits[1]) < valueOf(compareDigits[1]))
            return true;
        else {
            // Exact same behaviour as above, use private method
            if (valueOf(versionDigits[1]) == valueOf(compareDigits[1]))
                return valueOf(versionDigits[2]) < valueOf(compareDigits[2]);
            return false;
        }
    }
    // A lot of return statement, isn't there a simpler way?
    return false;
}
        

private boolean isPreviousVersion(String version, String compareWith) {
    String[] versionDigits = compare.split("\\.");
    String[] compareDigits = compareWith.split("\\.");
    for (int i = 0; i < versionDigits.length; i++) {
        if (isEquals(versionDigits[i], compareDigits[i])) {
            continue;
        }
        return isPrevious(versionDigits[i], compareDigits[i]);
    }
    return false;
}

private boolean isEquals(String versionNumber, String compareWith) {
    return Integer.parseInt(versionNumber) == Integer.parseInt(compareWith);
}

Simplicity wrap up

  • A method should be simple to read/understand
  •     When you're done coding a method, read yourself again
  • Beware of nested if and loops
  •     Bring complexity
  • Do not duplicate code, use private methods
  • You can use Sonar complexity metric

Example 3

Language and API usage


Collection<Object> removeDuplicates(Collection<?> collection) {
    List<Object> uniques = new ArrayList<>();
    for (Object object : collection)
        if (!uniques.contains(object))
            uniques.add((object));
    return uniques;
}

Date addDaysToDate(Date date, int numberOfDays) {
    Calendar calendar = Calendar.getInstance();
    calendar.setTime(date);
    calendar.add(Calendar.DAY_OF_YEAR, numberOfDays);
    return calendar.getTime();
}
        

Collection<Object> removeDuplicates(Collection<?> collection) {
    // You could use a set to remove duplicates
    List<Object> uniques = new ArrayList<>();
    for (Object object : collection)
        if (!uniques.contains(object))
            uniques.add(object);
    return uniques;
}

Date addDaysToDate(Date date, int numberOfDays) {
    // You could use the new Java 8 LocalDate API
    Calendar calendar = Calendar.getInstance();
    calendar.setTime(date);
    calendar.add(Calendar.DAY_OF_YEAR, numberOfDays);
    return calendar.getTime();
}
        

Collection<Object> removeDuplicates(Collection<?> collection) {
    return new HashSet<>(collection);
}

LocalDate addDaysToDate(LocalDate date, int numberOfDays) {
    return date.plus(numberOfDays, DAYS);
}

Language and API wrap up

  • Language usage should be short, simple, explicit
  • Know your language and data structures inside out:
  •     java.util.Map#[putIfAbsent, computeIfAbsent] (Java 8)
  •     java.time.[LocalDate, DateTimeFormatter] (Java 8)
  •     java.util.concurrent.[ExecutorService, CompletableFuture] (Java 5, 8)
  •     etc...
  • Some of it can be automated:
  •     IDEs, Sonar have simplification warnings that can help

Example 4

Object Oriented Programming (OOP)


public BigDecimal recomputePrice(Quote quote) throws ComputationPriceException {
  BigDecimal quoteTotalPrice = new BigDecimal(quote.getPrice().doubleValue());
  if (quote.getWarrantyAvailability1().equals(quote.getQuoteType()))
      quoteTotalPrice = quoteTotalPrice.add(quote.getWarrantyPrice1());
  if (quote.getWarrantyAvailability2().equals(quote.getQuoteType()))
      quoteTotalPrice = quoteTotalPrice.add(quote.getWarrantyPrice2());
  if (quote.getExpirationDate().isBefore(LocalDate.now()))
      throw new ComputationPriceException("It's too late !");
  if (!quote.isFeesIncluded()) {
      if (quote.getOneTimeFees() != null)
          quoteTotalPrice = quoteTotalPrice.add(quote.getOneTimeFees());
      if (quote.getAnnualFees() != null)
          quoteTotalPrice = quoteTotalPrice.add(quote.getAnnualFees());
  }
  // Taxes has changed after 1st January 2015
  if (quote.getDate().isBefore(LocalDate.of(2015, 1, 1)))
      quoteTotalPrice = quoteTotalPrice.multiply(new BigDecimal(1.20));
  else
      quoteTotalPrice = quoteTotalPrice.multiply(new BigDecimal(1.196));
  // Rounding
  return quoteTotalPrice.setScale(2, BigDecimal.ROUND_HALF_UP);
}
    

public BigDecimal recomputePrice(Quote quote) throws ComputationPriceException {
  // Variables with numbers is rarely a good option, use a list instead?
  // Warranties look independant from the quote, extract in an object
  BigDecimal quoteTotalPrice = new BigDecimal(quote.getPrice().doubleValue());
  if (quote.getWarrantyAvailability1().equals(quote.getQuoteType()))
      quoteTotalPrice = quoteTotalPrice.add(quote.getWarrantyPrice1());
  if (quote.getWarrantyAvailability2().equals(quote.getQuoteType()))
      quoteTotalPrice = quoteTotalPrice.add(quote.getWarrantyPrice2());
  // Use fail fast that avoid executing the function if not necessary
  if (quote.getExpirationDate().isBefore(LocalDate.now()))
      throw new ComputationPriceException("It's too late !");
  // Encapsulate implementation, we don't need to know about it here
  if (!quote.isFeesIncluded()) {
      if (quote.getOneTimeFees() != null)
          quoteTotalPrice = quoteTotalPrice.add(quote.getOneTimeFees());
      if (quote.getAnnualFees() != null)
          quoteTotalPrice = quoteTotalPrice.add(quote.getAnnualFees());
  }
  // Same remark as above
  // Taxes has changed after 1st January 2015
  if (quote.getDate().isBefore(LocalDate.of(2015, 1, 1)))
      quoteTotalPrice = quoteTotalPrice.multiply(new BigDecimal(1.20));
  else
      quoteTotalPrice = quoteTotalPrice.multiply(new BigDecimal(1.196));
  // Rounding
  return quoteTotalPrice.setScale(2, BigDecimal.ROUND_HALF_UP);
}
    

public BigDecimal recomputePrice(Quote quote) throws ComputationPriceException {
  checkQuoteIsStillValid(quote.getExpirationDate());

  BigDecimal quoteTotalPrice = new BigDecimal(quote.getPrice().doubleValue())
                  .add(warrantyService.computeWarrantiesAmount(quote.getWarranties()))
                  .add(feesService.computeFeesAmount(quote.getFees()));

  return taxesService.applyTaxes(quote.getDate(), quoteTotalPrice);
}

Object Oriented Programming class diagram

Object Oriented Programming wrap up

  • Encapsulation, inheritance, abstraction, polymorphism
  • No god object
  •     Don't be afraid of creating new classes
  • Do not duplicate classes for one special case
  •     Use inheritance or delegation accordingly
  • Structure your data in a smart way

References for more code review topics...

It's all about quality and communication

Why review?

  • Quality: uniformity, style, best practices
  • Communication: discussions, sharing, reduce project risk
  • Documentation: style guide used for discussion, list specific code smells
  • Bugs: prevent regressions, incomplete features

Who can review code?

  • Junior developers (simplicity)
  • Senior developers (oop / language)
  • Domain model expert developers (design / domain)
  • ... everybody!

Things we didn't talk about

Other code review topics: feature validation, units tests and code coverage, anti-pattern and code smells

Other review types: reviews during development or reviews by group, we don't do it but it works too!

Bonus: make sure you understand every line of code, review code not people (no shaming), do pair programming

Even Blindard does code review...

... you should do it too!