What's Clean Code?

Clean Code

• Code that's easy to understand ...
• ... even if the person that wrote it is not at your
  side (it could be you from some months ago)

Why is Clean Code important?

• "Unclean Code can bring an organization
  to its knees" -- Robert C. Martin

What could happen
if code is not clean?

• Your velocity on a
  project can be hampered
• Diminishing velocity
  could cancel your project
• A cancelled project
  may force layoffs

Clean Code is a
very serious matter

How can I write
Clean Code?

Care about the code you write

Care about your code

• It's not only about "my code works".
• Once it works, make it clean
• Write Code you could be proud of.

Be aware of the
Broken Window syndrome!

Use the Boy Scout rule:
commit code that's cleaner
than the one you pulled

What about performance?

It's true that more readable code usually comes with performance degradation

Clean Code Performance

• Usually small
• Other suspects
• First: Measure, Measure, Measure

Premature Optimization
is the root of all evil
-- Donald Knuth

Readability trumps performance
-- Martin Odersky

Recipes to
Clean Code

The key to
Clean Code is: methods

Methods

• should be short
• do one thing
• should be easy to test

What "short" really means?

All lines of code in a method should be at the same
level of abstraction

If I ask you what are your thoughts on the last
movie you watched...

... your answer
will be high level: "It was OK", "It was the worst ever", "It was better than expected", etc.

You wouldn't tell me about the acting, or the plot, or the length, etc. I will need to ask more questions.

We should strive
for public methods that
read more like prose

Those public methods should call other non-public methods, creating level of abstractions until the last level revels implementation details

Don't you think is possible?

1. Create a list of all numbers upto n
2. Cross 0 and 1
3. Call p the next uncrossed number
4. Step into every multiple of p in
   the list and cross it
5. Repeat step 3 and 4 until you have
   processed all numbers
6. Uncrossed numbers are prime

To find the prime numbers less than n:

How do you code it?

public class SievePrimeFinder implements PrimeFinder {

   // more code

   public SievePrimeFinder(final int max) {
      initSieve(max);
      uncrossAllNumbers();
      crossObviousNumbers();
      crossMultiplesOfKnownPrimes();
   }

   public boolean isPrime(final int n) {
      return isUncrossed(n);
   }
}

And then the methods at
the next level of abstraction

private void crossObviousNumbers() {
   cross(0);
   cross(1);
}

private void crossMultiplesOfKnownPrimes() {
   IntStream.range(2, this.sieve.length)
            .filter(SievePrimeFinder::isPrime)
            .forEach(SievePrimeFinder::crossMultiplesOf);
}

private void crossMultiplesOf(final int n) {
   // Java 9's iterate
   IntStream.iterate(firstMultiplePossiblyNotCrossed(n), 
                     x -> x < this.sieve.length, 
                     x -> x = nextMultiple(x, n))
            .forEach(SievePrimeFinder::cross);
}

And finally, the methods
with the lowest
level of abstraction

private void initSieve(final int max) {
   this.sieve = new boolean[max + 1];
}

private void uncrossAllNumbers() {
   Arrays.fill(this.sieve, true);
}

private void cross(int n) {
   this.sieve[n] = false;
}

private boolean isNotCrossed(final int n) {
   return this.sieve[n];
}
	
private int firstMultiplePossiblyNotCrossed(final int n) {
   return n * n;
}

private int nextMultiple(int lastMultiple, int n) {
   return lastMultiple + n;
}

Bowling Game

1. You play 10 frames
2. If you knock all pins in one try,
   you do a Strike in the current frame
3. If you knock all pins in two tries,
   you do a Spare in the current frame
4. If you do a Strike the points of that frame are 10 plus a big bonus (next two rolls)
5. If you do a Spare the points of that frame are 10 plus a bonus (next roll)
6. If you do neither the points of that frame are the number of pins you knocked after the 2 tries

public class BowlingGame {

  // more code

  public int score() {
    int score = 0;
    FrameManager frame = new FrameManager();
    while (frame.hasNext()) {
      if (frame.isStrike()) {
        score += 10 + frame.strikeBonus();
      } else if (frame.isSpare()) {
        score += 10 + frame.spareBonus();
      } else {
        score += frame.knockedPinsInFrame();
      }
      frame.advanceFrame();
    }
    return score;
  }
}

Minesweeper

• You have a rectangular board with cells
• A cell may have a mine, also they might contain a number which is the number of mines adjacent to it
• Cells are covered at the start
• You uncover a cell
• If the cell has a mine, you lose
• If not, you also uncover the neighbors of the cell until you reach a cell with a clue

public class MinesweeperBoard {
   // more code

   public void uncover(final int x, final int y) {
      if (hasMine(x, y)) {
         makeAllCellsVisible();
	 this.active = false;
         return;
      }
      recur(x, y);
   }

   private void recur(final int x, final int y) {
      if (inside(x, y) && isNotVisible(x, y)) {
         makeVisible(x, y);
	 if (isNotEmpty(x, y)) {
	    return;
         }
	 recur(x + 1, y);
	 recur(x - 1, y);
	 recur(x, y + 1);
	 recur(x, y - 1);
      }
   } 
}

Having levels of
abstraction have
some consequences that
could be seen as "negative"

Consequences

1. Classes are longer
2. You may have methods that
   are one-liners of very simple code

How can a longer class
be more readable?

1. Meaningful names + Javadocs
2. On average you'll read only method at the highest level

public class SievePrimeFinder implements PrimeFinder {
  
  private boolean[] sieve;

  public SievePrimeFinder(final int max) {
    this.sieve = new boolean[max + 1];
    Arrays.fill(this.sieve, true);
    this.sieve[0] = false;
    this.sieve[1] = false;
    for (int n = 2; n < this.sieve.length; ++n) {
      if (sieve[n]) {
        for (int x = n * n; x < this.sieve.length; x += n) {
	  sieve[x] = false;
	}
      }
    }
  }
  public boolean isPrime(int n) {
    return sieve[n];
  }
}

Do you think this is better?

What about performance
of one-liners?

Performance Myths

• Short methods, small gain
• Inlining

Exception-Safe methods

Exception-Safe Methods

• They should have only ONE try/catch
• If possible, there should be NO
  instructions before the try
• If possible, there should be NO
  instructions after the last catch

Having only one try/catch makes them easier to reason about, harder for bugs to hide

Beware of IDE's "surround with try/catch" operation!
That's what leads
to nested try / catch

If you put a tiny instruction before the try or after last catch, what could possibly go wrong?

It's a broken window!

String foo() {
   String message = "Hello World!";
   try {
      message = message.substring(0, 5);
   } catch (Exception e) { // don't do this
      message = "";
   }
   return message;
}

What do you think
of this code?

That looks wrong, isn't?

String foo() {
   try {
      String message = "Hello World!";
      return message.substring(0, 5);
   } catch (Exception e) { // don't do this
      return "";
   }
}

Much better!

Not related to Clean Code
but worth mentioning

• NEVER catch Exception
• Catch only the exceptions that you
  know can be thrown in the code
• NEVER throw Exception
• Throw an Unchecked Exception
  if it's a code error otherwise
  throw a Checked Exception
• Don't use exceptions for flow control

Honest Methods

If your method says that
you are returning a Foo,
then return a Foo!

null is not a Foo!

Returning null is very,
very error-prone!

Return an empty collection
or an Optional<Foo> to
signal absence of a value

Use Optional or one of the third-parties alternatives!

Don't accept null
as a parameter!

And don't even think about using Optional as a parameter!

public T doSomething(Foo foo, Bar bar) {
   if (bar == null) {
     // create a T only with foo
   } else {
     // create a T with both
   }
}

Instead of

public T doSomething(Foo foo) {
  // create a T only with foo
}

public T doSomethingAlsoWithABar(Foo foo, Bar bar) {
  // create a T with a foo and a bar
}

Do this

public T doSomething(Foo foo) {
  // create a T only with foo
}

public T doSomething(Foo foo, Bar bar) {
  // create a T with a foo and a bar
  // name doesn't explain how a bar will be used
}

Use method overloading judiciously

Be wary of code that use Optional's get() method,
use orElse-like methods

Don't go overboard
with Optional; use it

only as a return type

Looping

Replace loops
with Streams

public static double compute(int n, int k) {
  int number = n;
  int count = 1;
  double total = 0.0;
  while (count <= k) {
    if (primeFinder.isPrime(number)) {
      total += Math.sqrt(number);
      ++count;
    }
    ++number;
  }
  return total;
}

Can you guess what this does?

public static double compute(int n, int k) {
  return Stream.iterate(n, number -> number + 1)
               .filter(PrimeFinder::isPrime)
               .mapToDouble(Math::sqrt)
               .limit(k)
               .sum();
}

How about this?

FP forms tend to be
much more readable and introduce a level of abstraction

When refactoring loops into Streams, sometimes it's worth rethinking what you are
trying to accomplish

public List<List<String>> split(List<String> input) {
   List<List<String>> result = new ArrayList<>();
   int start = 0;
   for (int cur = 0; cur < input.size(); ++cur) {
      if (input.get(cur) == null) {
         result,add(input.subList(start, cur));
         start = cur + 1;
      }
   }
   result.add(input.subList(start, cur));
   return result;
}

Input [A, B, null, C, null, D]
Result [[A, B], [C], [D]]

private int[] getEdges(final List<String> input) {
  return IntStream.range(-1, input.size() + 1)
	          .filter(i -> i == -1)
	          .filter(i -> i == input.size())
	          .filter(i -> input.get(i) == null)
	          .toArray();
}

public List<List<String>> split(final List<String> input) {
  int[] edges = getEdges(input);
  return IntStream.range(0, edges.length - 1)
	          .mapToObj(k -> input.subList(edges[k]+1, edges[k+1]))
	          .collect(Collectors.toList());
}

Introduce levels of abstraction!

Double check for enhancements that your
IDE will probably miss

Lambda Expressions

Lambda Expressions

• Should be one-liners:
  have one sentence or call a method
• Improves readability and testability

Parameters

Parameters

• Try to avoid a lot of parameters...
• ... no more than 3, it makes your
  API harder to remember how to use

Parameters

• Use types to your advantage!
• They enforce invariants that
  can avoid possible errors

User findByName(String firstName, String lastName);

User findByName(Name name);

Instead of

Write

User find(String firstName, String lastName, String id);

User find(UserSearchCriteria criteria);

Instead of

Write

public void execute(String query);

public void execute(Query query); // Query's already sanitized

Instead of

Write

Types

• The most underused and undervalued
  feature of OOP languages
• Express our intentions better
  (String vs Name, String vs Query, etc)
• Enforce invariants, make
  some errors impossible

Immutability

Prefer Immutable Objects

• Code that mutates an
  object or that calls a method
  where an argument is
  mutated is hard to reason
• It's hard to reason about a
  class if its fields are mutable

Don't return a
mutable collection

• Returned collections should be
  used for read-only operations
• Return a Stream or an Immutable view of it

Immutability

• Learn about its in's and out's
• It doesn't always have a big
  impact on performance