Sunday, 6 March 2011

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");

 
}

}

No comments: