Refactoring Code - An Example


package refactoring;

/**
* This little tip was inspired by the book "Beginning EJB 3 Application Development" by Kodali
* and Whetherbee published by aPress. The code is the method aVeryBadMethod() tries to
* Initialize a simple wine by country database. However, IT IS JUST PLAINLY WRONG. Were they
* being deliberately stupid? Or just plain lazy? Or just trying to prove a point? I think it's
* appalling that code examples should be this wrong.
*
* This example takes that code and fixes it, but then goes through the possible options for
* making it better, and why you should choose them.
*
*
*/

public class WineClass {

  /** This won't work */
  private final Map<String, String> map = new HashMap<String, String>();

  /** The very least that could work */
  private final Map<String, String[]> map1 = new HashMap<String, String[]>();

  /** Slightly better */
  private final Map<String, List<String>> map2 = new HashMap<String, List<String>>();

  /** This is less error prone */
  private final Map<WineCountry, List<String>> map3 = new HashMap<WineCountry, List<String>>();

  /** This is less error prone */
  private final Map<WineCountry, List<Wine>> map4 = new HashMap<WineCountry, List<Wine>>();

  /**
   * This is an implementation of a wine-service. It makes the EJB class a facade and, to aid
   * testing, would be better if dependency injection was used. See other coding tips for an
   * example.
   */
  private final WineService wineService = new WineServiceImpl();

  /**
   * This is an example of a second wine-service implementation, which maybe considered
   * overkill by some It makes the EJB class a facade, and makes the return type an Object in
   * its own right rather than a Java collection (list) and, to aid testing, would be better
   * if dependency injection was used. See other coding tips for an example.
   */
  private final WineService2 wineService2 = new WineServiceImpl2();

  /**
   * This is code code from "Beginning EJB 3 Application Development" by Kodali
   *   and Whetherbee in Listing 2.6 in the method named:
   *
   *   
   *  @PostConstruct
   *  public void initializeCountryWineList()
   * 
   *  This is just WRONG. Even a novice programmer should know that maps don't work like this. 
   * 
   */
  public void aVeryBadMethod() {

    map.put("Australia", "Sauvignon Blanc");
    map.put("Australia", "Granache");
    map.put("France", "Gewurztraminer");
    map.put("France", "Bordeaux");
  }

  /**
   * This is the very least that the author could do to get the code working. But this is
   * still very messy because... well it uses arrays leading to things like
   * IndexOutOfBoundsException. The crux of the matter is that the data storage is tightly
   * coupled to the application by means of the array.
   */
  public void theVeryLeast() {

    map1.put("Australia", new String[] { "Sauvignon Blanc", "Granache" });
    map1.put("France", new String[] { "Gewurztraminer", "Bordeaux" });
  }

  /**
   * This is better. The code is working and we've decoupled the data storage responsibility
   * from the app and put it into the List. But this is still very messy because:
   *
   * 1) Writing Map<String,List<String>> is difficult to read and confusing
   * 2) Strings are still used throughout the code to represent finite sets of data.
   * 3) Strings are error prone and will fail silently.
   *
   */
  public void thisIsBetter() {

    List<String> list = new ArrayList<String>();
    list.add("Sauvignon Blanc");
    list.add("Granache");
    map2.put("Australia", Collections.unmodifiableList(list));

    list = new ArrayList<String>();
    list.add("Gewurztraminer");
    list.add("Bordeaux");
    map2.put("France", Collections.unmodifiableList(list));

  }

  /**
   * Remove one of the dependencies on the String class by using enums.
   */
  public static enum WineCountry {
    /** Wine from France */
    FRANCE,
    /** Wine from Australia */
    AUSTRALIA;
  }

  /**
   * Using an enum as the key means that we get rid of the error prone string. If a key is
   * wrong then we know about it at compile time.
   */
  public void betterStill() {
    List<String> list = new ArrayList<String>();
    list.add("Sauvignon Blanc");
    list.add("Granache");
    map3.put(WineCountry.AUSTRALIA, Collections.unmodifiableList(list));

    list = new ArrayList<String>();
    list.add("Gewurztraminer");
    list.add("Bordeaux");
    map3.put(WineCountry.FRANCE, Collections.unmodifiableList(list));
  }

  /**
   * Even so, we still depend upon the wine name as a String and this is also a finite set.
   * In this case, however the member of the set can change over time (I guess that the
   * number of countries producing wine could change too, but it's more unlikely). Hence, in
   * this can we can improve the typing by using a value object (or Bean or DTO, call it what
   * you will), whilst at the same time adding in the ability to add extra information to the
   * wine details. Also note that using a value object allows you to more easily change interfaces at
   * some future time.
   */
  public void couldWeDoBetter() {

    List<Wine> list = new ArrayList<Wine>();
    list.add(new Wine("Sauvignon Blanc", "South-West France", 12.4));
    list.add(new Wine("Granache", "Provance", 11.3));
    map4.put(WineCountry.AUSTRALIA, Collections.unmodifiableList(list));

    list = new ArrayList<Wine>();
    list.add(new Wine("Gewurztraminer", "Alsace", 12.3));
    list.add(new Wine("Bordeaux", "West France", 10.6));
    map4.put(WineCountry.FRANCE, Collections.unmodifiableList(list));

  }

  /**
   * The EJB3 code could be improved even more if we choose to use the facade pattern and
   * delegate the implementation of the wine-by-country service to... well a service. This,
   * if we then choose dependency injection will aid testing, in so far as we can inject
   * Mocks and Stubs and deploy / deliver test versions of the code.
   */
  public void isThisBetterStill() {
    wineService.initialise();
  }

  /**
   * This improves the wine-service, by hiding the storage method used. This is done by
   * simply returning an object of our own choosing that implements Iterator<T>. You now need
   * to consider whether this is overkill. We've gone from a simple List<String> storage type
   * to a full blown WineList<Wine> type that is part of a facade and totally hides its
   * implementation from the world. Remember that novice programmers may struggle with the
   * full-blown version, as safe as it is, and for all I know it may perform a lot slower
   * that the Map<String,List<String>> version (I'll bet it doesn't). So we may now be in a
   * trade off situation against using a more complex idea and fellow developers who may/may
   * not understand it. Remember that if this is not understood, then it'll get broken the
   * first time someone modifies it.
   *
   * Could we make it better by making it hard to break???
   *
   */
  public void isThisBetterOrOverKill() {
    wineService2.initialise();
  }

  /**
   * This is a simple wine object. As time goes on we can add in extra information about the
   * wine without breaking (substantially) any API contract
   *
   */
  public static class Wine implements Serializable {
    /**
     * Change this as and when - required for storing to disk and remoting on a Webserver
     * such as Weblogic.
     */
    private static final long serialVersionUID = 1L;
    private final String name;
    private final String orgin;
    private final double strength;

    /**
     * Create a wine object
     *
     * @param name
     *            The name of the wine
     * @param origin
     *            Where it's from
     * @param strength
     *            alcoholic strength
     */
    public Wine(String name, String origin, double strength) {
      this.name = name;
      this.orgin = origin;
      this.strength = strength;
    }

    /**
     * @return the name
     */
    public String getName() {
      return name;
    }

    /**
     * @return the orgin
     */
    public String getOrgin() {
      return orgin;
    }

    /**
     * @return the strength
     */
    public double getStrength() {
      return strength;
    }

  }

  /**
   * This is a definition of a POJO based service.
   *
   */
  private static interface WineService {

    /**
     * Initialize the wine service
     */
    void initialise();

    /**
     * Return a wine list for a given country.
     *
     * @param country
     *            The country
     * @return List this of wine dtos.
     */
    List<Wine> getWineByCountry(WineCountry country);
  }

  /**
   * As stated above, the example was taken from an EJB3 book. When developing EJBs it's
   * customary to use the facade pattern to disassociate the EJB from its implementation. The
   * aim here is to provide better re-use of POJO type code. However, you should consider
   * whether or not this approach carries as much validity with EJB 3 as it does in EJB 2.x. After all, EJB3 is
   * supposed to be a lightweight POJO based system. Having, said that using the facade
   * pattern allows delegation from one EJB to a number of services (or sub-services) thus
   * avoiding the 'bottleneck' anti-pattern, which to me is one of its main benefits.
   *
   * The 'Bottleneck' anti-pattern is something that I thought up and just a definition of how
   * a 'God' class or 'monster' class develops. This anti-pattern occurs when a dev-team doesn't
   * produce an appropriate object model for a system (lack of design) and then decides that
   * it must expose an interface. This interface is implemented by just one class. As time
   * goes on this class grows and grows, more methods are added to the interface, more and
   * different team members modify the class. It becomes a pile of inefficient spaghetti
   * code. The fix here would be to do some object design on the bottle-neck class, turning
   * it into a facade and delegating to a number of smaller services each with their
   * appropriate responsibilities.
   *
   * And speaking of bottlenecks, it wouldn't be beyond the realm of possibility for this
   * WineServiceImpl to become a bottle-neck. Care must be taken when developing services to
   * avoid the creation of God classes.
   *
   *
   */
  private static class WineServiceImpl implements WineService {

    /** This is less error prone */
    private final Map<WineCountry, List<Wine>> map = new HashMap<WineCountry, List<Wine>>();

    /**
     * @see refactoring.WineClass.WineService#initialise()
     *
     */
    @Override
    public void initialise() {
      List<Wine> list = new ArrayList<Wine>();
      list.add(new Wine("Sauvignon Blanc", "South-West France", 12.4));
      list.add(new Wine("Granache", "Provance", 11.3));
      map.put(WineCountry.AUSTRALIA, Collections.unmodifiableList(list));

      list = new ArrayList<Wine>();
      list.add(new Wine("Gewurztraminer", "Alsace", 12.3));
      list.add(new Wine("Bordeaux", "West France", 10.6));
      map.put(WineCountry.FRANCE, Collections.unmodifiableList(list));
    }

    /**
     * @see refactoring.WineClass.WineService#getWineByCountry()
     *
     */
    @Override
    public List<Wine> getWineByCountry(WineCountry country) {

      return Collections.unmodifiableList(map.get(country));
    }

  }

  /**
   * This is a defninition of a POJO based service.
   *
   * @author Roger
   *
   */
  private static interface WineService2 {

    /**
     * Initialise the wine service
     */
    void initialise();

    /**
     * Return a wine list for a given country.
     *
     * @param country
     *            The country
     * @return List this of wine dtos.
     */
    WineList getWineByCountry(WineCountry country);
  }

  private static class WineList implements Iterable<Wine> {

    private final Collection<Wine> fineWines;
    private final WineCountry country;

    public WineList(Collection<Wine> c, WineCountry country) {
      fineWines = c;
      this.country = country;
    }

    /**
     * @see java.lang.Iterable#iterator()
     *
     * @return Return an iterator to our wine list for this particular country. Note that
     *         this is made unmodifiable on construction.
     */
    @Override
    public Iterator<Wine> iterator() {
      return fineWines.iterator();
    }

    /**
     *
     * @return The wine country
     */
    @SuppressWarnings("unused")
    public WineCountry getCountry() {
      return country;
    }

  }

  /**
   * Create a service where the return type from the map doesn't rely on Java's collection
   * types. This gives us the ability to decide at some future time, the best collection type
   * to use. In this case, we've decided that a Set would be a better storage type, rather
   * than a list - but we could use a List.
   */
  private static class WineServiceImpl2 implements WineService2 {

    /** This is less error prone */
    private final Map<WineCountry, WineList> map = new HashMap<WineCountry, WineList>();

    /**
     * @see refactoring.WineClass.WineService#initialise()
     *
     */
    @Override
    public void initialise() {

      Set<Wine> set = new LinkedHashSet<Wine>();
      set.add(new Wine("Sauvignon Blanc", "Queensland", 12.4));
      set.add(new Wine("Granache", "Victoria", 11.3));
      map.put(WineCountry.AUSTRALIA, new WineList(Collections.unmodifiableSet(set),
          WineCountry.AUSTRALIA));

      set = new LinkedHashSet<Wine>();
      set.add(new Wine("Gewurztraminer", "Alsace", 12.4));
      set.add(new Wine("Bordeaux", "West France", 11.3));
      map.put(WineCountry.FRANCE, new WineList(Collections.unmodifiableSet(set),
          WineCountry.FRANCE));
    }

    /**
     * @see refactoring.WineClass.WineService#getWineByCountry()
     *
     */
    @Override
    public WineList getWineByCountry(WineCountry country) {

      return map.get(country);
    }

  }

  /**
   * Test the program
   *
   * @param args
   *            Not used
   */
  public static void main(String[] args) {

    System.out.println("WineClass start");
    WineClass test = new WineClass();

    test.aVeryBadMethod();

    test.theVeryLeast();

    test.thisIsBetter();

    test.betterStill();

    test.couldWeDoBetter();

    test.isThisBetterStill();

    test.isThisBetterOrOverKill();

    System.out.println("WineClass end");

  }

}