friendly-coins, Stage 1: Only one level of indentation per method

For the friendly-coins project, my strategy is to start from a completed project, and apply refactorings to arrive at a codebase obeying the Nine Rules.

I think this will be interesting for at least a couple reasons:

• It’ll give me a chance to see which of the Nine I tend to diverge from the most.
• I’ll be able to do a before/after measurement on a few code metrics (for example, complexity and cohesion).

The first refactor I applied was “Use only one level of indentation per method”.

Micro-state sharing

The bulk of the changes involved moving loops into their own methods.

I noticed a two variants of a micro-pattern. First, let’s look at a stringJoin (yes, this is available elsewhere — but I decided not to use any non-core libraries, and even to not use core implementations if writing the code might exercise some of the Nine Rules).

BEFORE:

public static String stringJoin( Collection objects, String joiner ) {
    StringBuilder builder = new StringBuilder();
    boolean first = true;
    for( Object o : objects ) {
        if( ! first ) {
            builder.append( joiner );
        } else {
            first = false;
        }
        builder.append( (null == o) ? "null" : o.toString() );
    }
    return builder.toString();
}

AFTER:

public static String stringJoin( Collection objects, String joiner ) {
    StringBuilder builder = new StringBuilder();
    String useJoiner = "";
    for( Object o : objects ) {
        first = stringJoinAppend( o, useJoiner, builder );
        useJoiner = joiner;
    }
    return builder.toString();
}

private static boolean stringJoinAppend( Object o, String joiner, StringBuilder builder ) {
    builder.append( joiner );
    builder.append( (null == o) ? "null" : o.toString() );
}

Now, let’s look at a method which find a minimal value (yes, yes, here I am reimplementing the wheel – in the interest of calisthenics).

BEFORE:

private CoinSet findMin(Collection coinSets) {
     CoinSet theLeast = null;
     for( CoinSet coinSet : coinSets ) {
         if( null == theLeast || coinSet.getNumCoins() < theLeast.getNumCoins() ) {
             theLeast = coinSet;
         }
     }
     return theLeast;
 }

AFTER:

private CoinSet findMin(Collection coinSets) {
     CoinSet theLeast = null;
     for( CoinSet coinSet : coinSets ) {
         theLeast = isItLess( theLeast, coinSet );
     }
     return theLeast;
 }

 private CoinSet isItLess(CoinSet theLeast, CoinSet coinSet) {
     if( null == theLeast || coinSet.getNumCoins() < theLeast.getNumCoins() ) {
         theLeast = coinSet;
     }
     return theLeast;
 }

I am claiming this is a “micropattern” characterized by the fact that there is state associated with the loop, and the state needs to be managed by coordination between the caller and callee methods. In the case of stringJoin, the state is whether or not to append the spearator to the string. In the case of findMin (which actually has a more descriptive name in the real source), it’s what the minimal value found so far is.

How the state is managed between caller and -ee differs in the two examples. In the stringJoin case, the state is independent of the called method (i.e., the caller maintains state). In the findMin case, the called method is responsible for maintaining the correct state. It would be possible to re-implement either method so that the responsibility is flipped. It’s just that the way I implemented both of them smelled right to me on the day I did it. It is worth noting that in the stringJoin, I have a redundant assignment (the assignment need only be done on the first pass through the loop). However, I liked getting rid of the “if” test that would be necessary. On the other hand, finding the minimal value does need a conditional, so putting the “if” in the callee works nicely.

Try / Catch – Problematic

Tricky to eliminate extra indentation were the try/catches – in this case, I took the requirement to be that there be 0 levels if indent inside the try/catch.  These are almost like a loop-removal, but seem to me to be more “woven” into the rest of the method (because of dependencies on variable).  This particular refactor smelled a little bad to me. I’d even say that try/catches might by their very nature introduce extra complexity that requires extra effort in implementation.

Strategy Pattern / Anonymous Methods / Yield / Micro-IOP

Also interesting, was one instance of a “collect” pattern (as in, the Ruby language “collect” method, which in other languages might be called “map”):

BEFORE:

@Override public String toString() {
      String ret = this.getClass().getSimpleName() + "<";
      boolean first = true;
      for( int k : denominations.keySet() ) {
          if( ! first ) {
              ret += ",";
          } else {
              first = false;
          }
          ret += k;
          ret += "'s:";
          ret += denominations.get( k );
      }
      return ret + ">";
      items.add( item );
  }

I tried to get tricky and introduce a first-class “foreach” in order to use the already-refactored stringJoin. The idea is that the difference is what gets executed INSIDE the foreach – so, since Java in its Old Skool way doesn’t yet have real anonymous functions, I tried to make one.

AFTER:

public String toString() {
      final Collection