Docstoc

Refactoring

Document Sample
Refactoring Powered By Docstoc
					Refactoring
                   A Story
• A 3rd party, such as a consultant, is asked to
  look at a project.
• He discovers that there are issues, such as a
  base class that is used inconsistently, etc.
• He advises spending two days to clean up the
  code.
• The project manager says that they can’t afford
  two days, because then they will miss their
  schedule.
• What should they do?
                 A quote
• “I didn’t have enough time to make it
  simpler.”
• What does this mean? Why would
  someone say this?
        What is refactoring?
• Refactoring is changing a software system
  such that it does not change the external
  behavior of the code, yet improves its
  internal structure.
• How your code looks/reads is important.
               An Example
• A program to calculate a print statement of a
  customer’s charges at a video store.
• Each customer may rented a number of movies
  for a certain period of time.
• The program calculates the charges depending
  on the type of movie: regular, children’s, new
  releases.
• Also have frequent renter points, depending on
  whether or not the movie is a new release.
• (Diagram)
                                                  public class Rental {
public class Movie {                                private Movie _movie;
                                                    private int _daysRented;
    public static final int CHILDRENS = 2;          public Rental(Movie movie, int daysRented) {
    public static final int REGULAR = 0;              _movie = movie;
    public static final int NEW_RELEASE = 1;          _daysRented = daysRented;
                                                    }
    private String _title;                          public int getDaysRented() {
    private int _priceCode;                           return _daysRented;
                                                    }
    public Movie(String title, int priceCode) {     public Movie getMovie() {
      _title = title;                                 return _movie;
      _priceCode = priceCode;                       }
    }                                             }
    public int getPriceCode() {
      return _priceCode;
    }                                              public class Customer {
    public void setPriceCode(int arg) {              private String _name;
      _priceCode = arg;                              private Vector _rentals = new Vector();
    }                                                public Customer(String name) {
    public String getTitle() {                         _name = name;
      return _title;                                 }
    }                                                public void addRental(Rental arg) {
}                                                      _rentals.addElement(arg);
                                                     }
                                                     public String getName() {
                                                       return _name;
                                                     }
                                                     public String statement() { … }
                                                   }
public String Customer.statement() {                             // Add frequent renter points.
 double totalAmount = 0;                                         frequentRenterPoints++;
 int frequentRenterPoints = 0;                                   // Add bonus for a 2-day new release rental.
                                                                 if (each.getMovie().getPriceCode()
 Enumeration rentals = _rentals.elements();                                            == Movie.NEW_RELEASE)
 String result = “Rental Record for “ + getName() + “\n”;                            && each.getDaysRented() > 1)
 while (rentals.hasMoreElements()) {                               frequentRenterPoints++;
  double thisAmount = 0;                                         // Show figures for this rental.
  Rental each = (Rental) rentals.nextElement();                  result += “\t” + each.getMovie().getTitle() + “\t”
                                                                               + String.valueOf(thisAmount) + “\n”;
  // Determine amounts for each line.                            totalAmount += thisAmount;
  switch (each.getMovie().getPriceCode()) {                     }
    case Movie.REGULAR:                                         // Add footer lines.
     thisAmount += 2;                                           result += “Amount owed is “
     if (each.getDaysRented() > 2)                                            + String.valueOf(totalAmount) + “\n”;
       thisAmount += (each.getDaysRented() – 2) * 1.5;          result += “You earned “
     break;                                                                + String.valueOf(frequentRenterPoints)
    case Movie.NEW_RELEASE:                                                + “ frequent renter points”;
     thisAmount += each.getDaysRented()*3;                      return result;
     break;                                                 }
    case Movie.CHILDRENS:
     thisAmount += 1.5;
     if (each.getDaysRented() > 3)
       thisAmount += (each.getDaysRented() – 3) * 1.5;
     break;
  }

    How is this? What if HTML output is desired? Different categories,
    diffferent points?
• First step: Create tests, preferably
  automatic. Should they be unit tests?
• How should it be improved?
public String Customer.statement() {                        Factor charge code out.
 double totalAmount = 0;
 int frequentRenterPoints = 0;
 Enumeration rentals = _rentals.elements();
 String result = “Rental Record for “ + getName() + “\n”;      // Add frequent renter points.
 while (rentals.hasMoreElements()) {                           frequentRenterPoints++;
   double thisAmount = 0;                                      // Add bonus for a 2-day new release rental.
   Rental each = (Rental) rentals.nextElement();               if (each.getMovie().getPriceCode()
                                                                                     == Movie.NEW_RELEASE)
  // Determine amounts for each line.                                              && each.getDaysRented() > 1)
  switch (each.getMovie().getPriceCode()) {                      frequentRenterPoints++;
    case Movie.REGULAR:                                        // Show figures for this rental.
     thisAmount += 2;                                          result += “\t” + each.getMovie().getTitle() + “\t”
     if (each.getDaysRented() > 2)                                           + String.valueOf(thisAmount) + “\n”;
       thisAmount += (each.getDaysRented() – 2) * 1.5;         totalAmount += thisAmount;
     break;                                              }
    case Movie.NEW_RELEASE:                              // Add footer lines.
     thisAmount += each.getDaysRented()*3;               result += “Amount owed is “
     break;                                                            + String.valueOf(totalAmount) + “\n”;
    case Movie.CHILDRENS:                                result += “You earned “
     thisAmount += 1.5;                                             + String.valueOf(frequentRenterPoints)
     if (each.getDaysRented() > 3)                                  + “ frequent renter points”;
       thisAmount += (each.getDaysRented() – 3) * 1.5; return result;
     break;                                            }
  }
public String Customer.statement() {
  double totalAmount = 0;                                 private double AmountFor(Rental each) {
  int frequentRenterPoints = 0;                             double thisAmount = 0;
  Enumeration rentals = _rentals.elements();                switch (each.getMovie().getPriceCode()) {
  String result = “Rental Record for “ + getName() + “\n”; case Movie.REGULAR:
  while (rentals.hasMoreElements()) {                          thisAmount += 2;
    double thisAmount = 0;                                     if (each.getDaysRented() > 2)
    Rental each = (Rental) rentals.nextElement();                thisAmount += (each.getDaysRented() – 2) * 1.5;
                                                               break;
    thisAmount = amountFor(each);                             case Movie.NEW_RELEASE:
                                                               thisAmount += each.getDaysRented()*3;
    // Add frequent renter points.                             break;
    frequentRenterPoints++;                                   case Movie.CHILDRENS:
    // Add bonus for a 2-day new release rental.               thisAmount += 1.5;
    if (each.getMovie().getPriceCode()                         if (each.getDaysRented() > 3)
                          == Movie.NEW_RELEASE)                  thisAmount += (each.getDaysRented() – 3) * 1.5;
                        && each.getDaysRented() > 1)           break;
      frequentRenterPoints++;                               }
    // Show figures for this rental.                        return thisAmount;
    result += “\t” + each.getMovie().getTitle() + “\t”    }
                  + String.valueOf(thisAmount) + “\n”;
    totalAmount += thisAmount;
  }
  // Add footer lines.
  result += “Amount owed is “ + String.valueOf(totalAmount) + “\n”;
  result += “You earned “ + String.valueOf(frequentRenterPoints) + “ frequent renter points”;
  return result;
}
           Refactoring changes the code in small steps. Why is this good?
private double Customer.AmountFor(Rental each) {         private double Customer.AmountFor(Rental aRental) {
  double thisAmount = 0;                                   double result = 0;
  switch (each.getMovie().getPriceCode()) {                switch (aRental.getMovie().getPriceCode()) {
    case Movie.REGULAR:                                      case Movie.REGULAR:
     thisAmount += 2;                                         result += 2;
     if (each.getDaysRented() > 2)                            if (aRental.getDaysRented() > 2)
       thisAmount += (each.getDaysRented() – 2) * 1.5;          result += (aRental.getDaysRented() – 2) * 1.5;
     break;                                                   break;
    case Movie.NEW_RELEASE:                                  case Movie.NEW_RELEASE:
     thisAmount += each.getDaysRented()*3;                    result += aRental.getDaysRented()*3;
     break;                                                   break;
    case Movie.CHILDRENS:                                    case Movie.CHILDRENS:
     thisAmount += 1.5;                                       result += 1.5;
     if (each.getDaysRented() > 3)                            if (aRental.getDaysRented() > 3)
       thisAmount += (each.getDaysRented() – 3) * 1.5;          result += (aRental.getDaysRented() – 3) * 1.5;
     break;                                                   break;
  }                                                        }
  return thisAmount;                                       return result;
}                                                        }




         Rename some variables.
         Good code should communicate what it’s doing. Writing code that
         humans can understand is often harder than writing code that
         computers can understand.
  Which class should this method be in? Does it use anything from the
  Customer object?


class Customer {
 private double AmountFor(Rental aRental) {
   double result = 0;
   switch (aRental.getMovie().getPriceCode()) {
     case Movie.REGULAR:
      result += 2;
      if (aRental.getDaysRented() > 2)
        result += (aRental.getDaysRented() – 2) * 1.5;
      break;
     case Movie.NEW_RELEASE:
      result += aRental.getDaysRented()*3;
      break;
     case Movie.CHILDRENS:
      result += 1.5;
      if (aRental.getDaysRented() > 3)
        result += (aRental.getDaysRented() – 3) * 1.5;
      break;
   }
   return result;
 }
  Move to Rental class.
  What should we do with the old method (amountFor()) in Customer?


class Rental {                                   class Customer {
 double getCharge() {                             private double amountFor(Rental aRental) {
   double result = 0;                               return aRental.getCharge();
   switch (getMovie().getPriceCode()) {           }
     case Movie.REGULAR:
      result += 2;
      if (getDaysRented() > 2)
        result += (getDaysRented() – 2) * 1.5;
      break;
     case Movie.NEW_RELEASE:
      result += getDaysRented()*3;
      break;
     case Movie.CHILDRENS:
      result += 1.5;
      if (getDaysRented() > 3)
        result += (getDaysRented() – 3) * 1.5;
      break;
   }
   return result;
 }
                                                              public String Customer.statement() {
public String Customer.statement() {                           …
 double totalAmount = 0;                                        thisAmount = each.getCharge();
 int frequentRenterPoints = 0;
 Enumeration rentals = _rentals.elements();                      // Add frequent renter points.
 String result = “Rental Record for “ + getName() + “\n”;         frequentRenterPoints++;
 while (rentals.hasMoreElements()) {                              // Add bonus for a 2-day new release rental.
   double thisAmount = 0;                                         if (each.getMovie().getPriceCode()
   Rental each = (Rental) rentals.nextElement();                                        == Movie.NEW_RELEASE)
                                                                                      && each.getDaysRented() > 1)
     thisAmount = amountFor(each);                                  frequentRenterPoints++;
                                                                  // Show figures for this rental.
     // Add frequent renter points.                               result += “\t” + each.getMovie().getTitle() + “\t”
      frequentRenterPoints++;                                                   + String.valueOf(thisAmount) + “\n”;
      // Add bonus for a 2-day new release rental.                totalAmount += thisAmount;
      if (each.getMovie().getPriceCode()                        }
                            == Movie.NEW_RELEASE)               // Add footer lines.
                          && each.getDaysRented() > 1)          result += “Amount owed is “
        frequentRenterPoints++;                                                  + String.valueOf(totalAmount) + “\n”;
      // Show figures for this rental.                          result += “You earned “
      result += “\t” + each.getMovie().getTitle() + “\t”               + String.valueOf(frequentRenterPoints)
                    + String.valueOf(thisAmount) + “\n”;                 + “ frequent renter points”;
      totalAmount += thisAmount;                                return result;
    }                                                           }
    // Add footer lines.                                             Use the new method on rental. Old
    result += “Amount owed is “                                      amountFor()?
                          + String.valueOf(totalAmount) + “\n”;
    result += “You earned “ + String.valueOf(frequentRenterPoints) + “ frequent renter points”;
    return result;
}
public String Customer.statement() {
 double totalAmount = 0;                                              What about temporary
 int frequentRenterPoints = 0;
                                                                      variables?
 Enumeration rentals = _rentals.elements();
 String result = “Rental Record for “ + getName() + “\n”;
 while (rentals.hasMoreElements()) {                          public String Customer.statement() {
   double thisAmount = 0;                                        …
   Rental each = (Rental) rentals.nextElement();                  // Add frequent renter points.
                                                                  frequentRenterPoints++;
     thisAmount = each.getCharge();                               // Add bonus for a 2-day new release rental.
                                                                  if (each.getMovie().getPriceCode()
     // Add frequent renter points.                                                     == Movie.NEW_RELEASE)
      frequentRenterPoints++;                                                         && each.getDaysRented() > 1)
      // Add bonus for a 2-day new release rental.                  frequentRenterPoints++;
      if (each.getMovie().getPriceCode()                          // Show figures for this rental.
                            == Movie.NEW_RELEASE)                 result += “\t” + each.getMovie().getTitle() + “\t”
                          && each.getDaysRented() > 1)                          + String.valueOf(each.getCharge()) + “\
        frequentRenterPoints++;                                   totalAmount += each.getCharge();
      // Show figures for this rental.                          }
      result += “\t” + each.getMovie().getTitle() + “\t”        …
                    + String.valueOf(thisAmount) + “\n”;      }
      totalAmount += thisAmount;
    }
    // Add footer lines.
    result += “Amount owed is “
                          + String.valueOf(totalAmount) + “\n”;
    result += “You earned “ + String.valueOf(frequentRenterPoints) + “ frequent renter points”;
    return result;
}
                                                                 Where does the calculation
                                                                 of frequent renter points go?
public String Customer.statement() {
 double totalAmount = 0;                                            public String Customer.statement() {
 int frequentRenterPoints = 0;                                        …
 Enumeration rentals = _rentals.elements();                           while (rentals.hasMoreElements()) {
 String result = “Rental Record for “ + getName() + “\n”;               Rental each = (Rental) rentals.nextElement();
 while (rentals.hasMoreElements()) {                                    frequentRenterPoints
   Rental each = (Rental) rentals.nextElement();                                       += each.getFrequentRenterPoints();
                                                                        // Show figures for this rental.
      // Add frequent renter points.                                    result += “\t” + each.getMovie().getTitle() + “\t”
     frequentRenterPoints++;                                                          + String.valueOf(each.getCharge) + “\n
     // Add bonus for a 2-day new release rental.                       totalAmount += each.getCharge();
     if (each.getMovie().getPriceCode()                               }
                         == Movie.NEW_RELEASE)                        …
                       && each.getDaysRented() > 1)                 }
       frequentRenterPoints++;

     // Show figures for this rental.                              class Rental { …
     result += “\t” + each.getMovie().getTitle() + “\t”             int getFrequentRenterPoints() {
                   + String.valueOf(each.getCharge) + “\n”;           if (getMovie().getPriceCode()
     totalAmount += each.getCharge();                                                      == Movie.NEW_RELEASE)
    }                                                                                   && getDaysRented() > 1)
    // Add footer lines.                                                return 2;
    result += “Amount owed is “                                       else
                         + String.valueOf(totalAmount) + “\n”;          return 1;
    result += “You earned “                                         }
       + String.valueOf(frequentRenterPoints)
       + “ frequent renter points”;
    return result;
}
public String Customer.statement() {                         What is being done in this loop? How
  double totalAmount = 0;                                    related are they? Unfuse loops.
  int frequentRenterPoints = 0;
  Enumeration rentals = _rentals.elements();
  String result = “Rental Record for “ + getName() + “\n”;
  while (rentals.hasMoreElements()) {                        class Customer { …
    Rental each = (Rental) rentals.nextElement();             public String statement() {
    frequentRenterPoints += each.getFrequentRenterPoints(); …
    // Show figures for this rental.                            while (rentals.hasMoreElements()) {
    result += “\t” + each.getMovie().getTitle() + “\t”            Rental each = (Rental) rentals.nextElement();
                  + String.valueOf(each.getCharge) + “\n”;        result += “\t” + each.getMovie().getTitle() + “\t”
    totalAmount += each.getCharge();                                    + String.valueOf(each.getCharge) + “\n”;
  }                                                             }
  // Add footer lines.                                          // Add footer lines.
  result += “Owed “ + String.valueOf(totalAmount) + “\n”;       result += “Amount owed is “
  result += “Earned “ + String.valueOf(frequentRenterPoints)         + String.valueOf(getTotalCharge()) + “\n”;
          + “ frequent renter points”;                          result += “You earned “
  return result;                                               + String.valueOf(getTotalFrequentRenterPoints())
}                                                              + “ frequent renter points”;
                                                                return result;
        class Customer { …
                                                              }
         private double getTotalCharge() {
                                                              private int getTotalFrequentRenterPoints() {
           double result = 0;
                                                                int result = 0;
           Enumeration rentals = _rentals.elements();
                                                                Enumeration rentals = _rentals.elements();
           while (rentals.hasMoreElements()) {
                                                                while (rentals.hasMoreElements()) {
             Rental each = (Rental) rentals.nextElement();
                                                                  Rental each = (Rental) rentals.nextElement();
             result += each.getCharge();
                                                                  result += each.getFrequentRenterPoints();
           }
                                                                }
           return result;
                                                                return result;
         }
                                                              }
class Customer { …
 public String statement() {                                               How hard is it to create an
   Enumeration rentals = _rentals.elements();                              HTML version now?
   String result = “Rental Record for “ + getName() + “\n”;
   while (rentals.hasMoreElements()) {
     Rental each = (Rental) rentals.nextElement();
     result += “\t” + each.getMovie().getTitle() + “\t”
                   + String.valueOf(each.getCharge) + “\n”;
   }
   // Add footer lines.
   result += “Amount owed is “
                       + String.valueOf(getTotalCharge()) + “\n”;
   result += “You earned “             class Customer { …
          + String.valueOf(getTotalFrequentRenterPoints())
                                         public String htmlStatement() {
          + “ frequent renter points”;     Enumeration rentals = _rentals.elements();
   return result;                          String result = “<h1>Rentals for <em>“ + getName() + “</em></h1><p>\n”;
 }                                         while (rentals.hasMoreElements()) {
                                             Rental each = (Rental) rentals.nextElement();
                                             result += “\t” + each.getMovie().getTitle() + “\t”
                                                           + String.valueOf(each.getCharge) + “<br>\n”;
                                           }
                                           // Add footer lines.
                                           result += “<p>You owe <em>“
                                                               + String.valueOf(getTotalCharge()) + “</em><p>\n”;
 Could even more be                        result += “You earned <em>“
 done?                                            + String.valueOf(getTotalFrequentRenterPoints())
                                                  + “</em> frequent renter points<p>”;
                                           return result;
                                         }
They want to change the       class Customer { …
                               public String statement() {
movie categories and rental      Enumeration rentals = _rentals.elements();
                                 String result = “Rental Record for “ + getName() + “\n”;
points.                          while (rentals.hasMoreElements()) {
                                   Rental each = (Rental) rentals.nextElement();
                                   result += “\t” + each.getMovie().getTitle() + “\t”
                                                 + String.valueOf(each.getCharge) + “\n”;
                                 }
                                 // Add footer lines.
                                 result += “Amount owed is “
                                                     + String.valueOf(getTotalCharge()) + “\n”;
                                 result += “You earned “
                                        + String.valueOf(getTotalFrequentRenterPoints())
                                        + “ frequent renter points”;
                                 return result;
                               }
                               private int getTotalFrequentRenterPoints() {
                                 int result = 0;
                                 Enumeration rentals = _rentals.elements();
                                 while (rentals.hasMoreElements()) {
                                   Rental each = (Rental) rentals.nextElement();
                                   result += each.getFrequentRenterPoints();
                                 }
                                 return result;
                               }
                               private double getTotalCharge() {
                                 double result = 0;
                                 Enumeration rentals = _rentals.elements();
                                 while (rentals.hasMoreElements()) {
                                   Rental each = (Rental) rentals.nextElement();
                                   result += each.getFrequentRenterPoints();
                                 }
                                 return result;
                               }
                                                 class Movie {
 getCharge() computes the price of                double getCharge(int daysRented) {
                                                    double result = 0;
 movie. Does this belong with Rental or
                                                    switch (getPriceCode()) {
 Movie class?                                         case Movie.REGULAR:
                                                       result += 2;
                                                       if (daysRented > 2)
                                                         result += (daysRented – 2) * 1.5;
class Rental {
                                                       break;
 double getCharge() {
                                                      case Movie.NEW_RELEASE:
   double result = 0;
                                                       result += daysRented*3;
   switch (getMovie().getPriceCode()) {
                                                       break;
     case Movie.REGULAR:
                                                      case Movie.CHILDRENS:
      result += 2;
                                                       result += 1.5;
      if (getDaysRented() > 2)
                                                       if (daysRented > 3)
        result += (getDaysRented() – 2) * 1.5;
                                                         result += (daysRented – 3) * 1.5;
      break;
                                                       break;
     case Movie.NEW_RELEASE:
                                                    }
      result += getDaysRented()*3;
                                                    return result;
      break;
                                                  }
     case Movie.CHILDRENS:
      result += 1.5;
      if (getDaysRented() > 3)                   class Rental {
        result += (getDaysRented() – 3) * 1.5;    double getCharge() {
      break;                                        return _movie.getCharge(_daysRented);
   }                                              }
   return result;
 }
 How about the calculation of frequent
                                                     class Movie { …
 renter points?                                       int getFrequentRenterPoints(int daysRented) {
                                                        if ((getPriceCode() == Movie.NEW_RELEASE)
                                                                              && daysRented > 1)
class Rental { …                                          return 2;
 int getFrequentRenterPoints() {                        else
   if (getMovie().getPriceCode()                          return 1;
                        == Movie.NEW_RELEASE)         }
                     && each.getDaysRented() > 1)
     return 2;
   else
     return 1;
 }
                                                    class Rental {
                                                     double getFrequentRenterPoints() {
                                                       return _movie.getFrequentRenterPoints(_daysRented);
                                                     }
We have three different types of movies.
We are using switches to return different
prices and use different rental point
calculations. Can we replace with                  class Movie {
                                                    double getCharge(int daysRented) {
polymorphism?                                         double result = 0;
                                                      switch (getPriceCode()) {
                                                        case Movie.REGULAR:
                                                         result += 2;
                                                         if (daysRented > 2)
  class Movie { …                                          result += (daysRented – 2) * 1.5;
   int getFrequentRenterPoints(int daysRented) {         break;
     if ((getPriceCode() == Movie.NEW_RELEASE)          case Movie.NEW_RELEASE:
                           && daysRented > 1)            result += daysRented*3;
       return 2;                                         break;
     else                                               case Movie.CHILDRENS:
       return 1;                                         result += 1.5;
   }                                                     if (daysRented > 3)
                                                           result += (daysRented – 3) * 1.5;
                                                         break;
                                                      }
                                                      return result;
                                                    }
Does this work?                              class RegularMovie : public Movie {
                                               double getCharge(int daysRented) {
                                                double result = 2;
                                                if (daysRented > 2)
                                                  result += (daysRented – 2) *1.5;
                                                return result;
                                             }
 class Movie {
  double getCharge(int daysRented) {
    double result = 0;
    switch (getPriceCode()) {
                                             class NewReleaseMovie : public Movie {
      case Movie.REGULAR:
                                               double getCharge(int daysRented) {
       result += 2;
                                                return daysRented*3;
       if (daysRented > 2)
                                             }
         result += (daysRented – 2) * 1.5;
       break;
      case Movie.NEW_RELEASE:
       result += daysRented*3;
       break;                                class ChildrensMovie : public Movie {
      case Movie.CHILDRENS:                    double getCharge(int daysRented) {
       result += 1.5;                           double result = 1.5;
       if (daysRented > 3)                      if (daysRented > 3)
         result += (daysRented – 3) * 1.5;        result += (daysRented – 3) *1.5;
       break;                                   return result;
    }                                        }
    return result;
  }
Use the state pattern on price.

Lets look again at the code. Remember
the goal is to replace the price int with
an object that represents the state of
the movie. Note that this could just be
simple composition.
public class Movie {                                  public class Customer {
                                                        private String _name;
    public static final int CHILDRENS = 2;              private Vector _rentals = new Vector();
    public static final int REGULAR = 0;                public Customer(String name) {
    public static final int NEW_RELEASE = 1;              _name = name;
                                                        }
    private String _title;                              public void addRental(Rental arg) { _rentals.addElement(arg); }
    private int _priceCode;                             public String getName() { return _name; }
                                                        private int getTotalFrequentRenterPoints() { … }
    public Movie(String title, int priceCode) {         private double getTotalCharge() { … }
      _title = title;                                   public String statement() { … }
      _priceCode = priceCode;                         }
    }

    public int getPriceCode() { ret _prcCde; }
    public void setPriceCode(int a) { _prcCd=a; }
    public String getTitle() { return _title; }
    public int frequentRenterPoints(int days) { … }
    public double getCharge(int days) { … }
}

public class Rental {
  private Movie _movie;
  private int _daysRented;
  public Rental(Movie movie, int daysRented) {
    _movie = movie;
    _daysRented = daysRented;
  }
  public int getDaysRented() { return _daysRented; }
  public Movie getMovie() { return _movie; }
}
Charge calculations.                                  public class Movie {

                                                          public static final int CHILDRENS = 2;
                                                          public static final int REGULAR = 0;
                                                          public static final int NEW_RELEASE = 1;
public class Rental { …
 private Movie _movie;                                    private int _priceCode;
 private int _daysRented;
 double getCharge() {                                  public int getPriceCode() { ret _prcCde; }
   return _movie.getCharge(_daysRented);              double getCharge(int daysRented) {
 }                                                       double result = 0;
                                                         switch (getPriceCode()) {
                                                           case Movie.REGULAR:
                                                            result += 2;
                                                            if (daysRented > 2)
                                                              result += (daysRented – 2) * 1.5;
                                                            break;
 public class Customer { …                                 case Movie.NEW_RELEASE:
  private double getTotalCharge() {                         result += daysRented*3;
    double result = 0;                                      break;
    Enumeration rentals = _rentals.elements();             case Movie.CHILDRENS:
    while (rentals.hasMoreElements()) {                     result += 1.5;
      Rental each = (Rental) rentals.nextElement();         if (daysRented > 3)
      result += each.getCharge();                             result += (daysRented – 3) * 1.5;
    }                                                       break;
    return result;                                       }
  }                                                      return result;
                                                       }
                                                      }
    Rental points
    calculations.                              public class Rental { …
                                                private Movie _movie;
                                                private int _daysRented;
                                                double getFrequentRenterPoints() {
                                                  return _movie.getFrequentRenterPoints(_daysRented);
                                                }
public class Movie {

 public static final int CHILDRENS = 2;
 public static final int REGULAR = 0;
 public static final int NEW_RELEASE = 1;

 private int _priceCode;
                                                public class Customer {
                                                  private int getTotalFrequentRenterPoints() {
  public int getPriceCode() { ret _prcCde; }
                                                    int result = 0;
  int getFrequentRenterPoints(int daysRented) {
                                                    Enumeration rentals = _rentals.elements();
    if ((getPriceCode() == Movie.NEW_RELEASE)
                                                    while (rentals.hasMoreElements()) {
                          && daysRented > 1)
                                                      Rental each = (Rental) rentals.nextElement();
      return 2;
                                                      result += each.getFrequentRenterPoints();
    else
                                                    }
      return 1;
                                                    return result;
  }
                                                  }
}
                                                }
 Now create a class
                                          abstract class Price {
 hierarchy for the price code.              abstract int getPriceCode();
                                          }
                                          class ChildrensPrice extends Price {
                                            int getPriceCode() { return Movie.CHILDRENS; }
                                          }
                                          class NewReleasePrice extends Price {
class Movie { …                             int getPriceCode() { return Movie.NEWRELEASE; }
 public Movie(String name, int price) {   }
   _title = name;                         class RegularPrice extends Price {
   setPriceCode(priceCode);                 int getPriceCode() { return Movie.REGULAR; }
 }                                        }




      Should the type codes still be part of movie, or in price?
 Change the accessor
                                           class Movie { …
 functions.                                 public int getPriceCode() {
                                              return _price.getPriceCode();
                                            }
                                            public void setPriceCode(int arg) {
                                              switch (arg) {
                                                case REGULAR:
                                                 _price = new RegularPrice();
                                                 break;
                                                case CHILDRENS:
class Movie { …                                  _price = new ChildrensPrice();
 public int getPriceCode() {                     break;
   return _priceCode;                           case NEWRELEASE:
 }                                               _price = new NewReleasePrice();
 public void setPriceCode(int arg) {             break;
   _priceCode = arg;                          }
 }                                          }
 private int priceCode;                     private Price priceCode;




      Should the price creation be in Movie or external?
abstract class Price {
  abstract double getCharge();
  int getFrequentRentersPoints() {
    return 1;
  }                                               class ChildrensPrice {
}                                                  double getCharge(int daysRented) {
class RegularPrice {…                                double result = 1.5;
  double getCharge(int daysRented) {                 if (daysRented > 3)
    double result = 2;                                 result += (daysRented – 3)*1.5;
    if (daysRented > 2)                              return result;
      result += (daysRented – 2) * 1.5;            }
    return result;
  }
class NewReleasePrice {
  double getCharge(int daysRented) {
    return daysRented *3;
  }
  int getFrequentRenterPoints(int daysRented) {
    return (daysRented > 1) ? 2 : 1;
  }
    Charge and rental
    calculations.

public class Movie {

    public static final int CHILDRENS = 2;
    public static final int REGULAR = 0;
    public static final int NEW_RELEASE = 1;

    private int _priceCode;

 public int getPriceCode() { ret _prcCde; }
double getCharge(int daysRented) {
   return _price.getCharge(daysRented);
 }

int getFrequentRenterPoints(int daysRented) {
  return _price.getFrequentRenterPoints();
}


}
class Customer { …
 public String statement() {                                              How else could we do this
   Enumeration rentals = _rentals.elements();                             one?
   String result = “Rental Record for “ + getName() + “\n”;
   while (rentals.hasMoreElements()) {                                          •Template method
     Rental each = (Rental) rentals.nextElement();
     result += “\t” + each.getMovie().getTitle() + “\t”                         •Builder
                   + String.valueOf(each.getCharge) + “\n”;
   }
   // Add footer lines.
   result += “Amount owed is “
                       + String.valueOf(getTotalCharge()) + “\n”;
   result += “You earned “           class Customer { …
          + String.valueOf(getTotalFrequentRenterPoints())
                                      public String htmlStatement() {
          + “ frequent renter points”; Enumeration rentals = _rentals.elements();
   return result;                       String result = “<h1>Rentals for <em>“ + getName() + “</em></h1><p>\n”;
 }                                      while (rentals.hasMoreElements()) {
                                          Rental each = (Rental) rentals.nextElement();
                                          result += “\t” + each.getMovie().getTitle() + “\t”
                                                        + String.valueOf(each.getCharge) + “<br>\n”;
                                        }
                                        // Add footer lines.
                                        result += “<p>You owe <em>“
                                                            + String.valueOf(getTotalCharge()) + “</em><p>\n”;
 Could even more be                     result += “You earned <em>“
 done?                                         + String.valueOf(getTotalFrequentRenterPoints())
                                               + “</em> frequent renter points<p>”;
                                        return result;
                                      }
class Customer { …
 private double getTotalCharge() {                   private int getTotalFrequentRenterPoints() {
   double result = 0;                                   int result = 0;
   Enumeration rentals = _rentals.elements();           Enumeration rentals = _rentals.elements();
   while (rentals.hasMoreElements()) {                  while (rentals.hasMoreElements()) {
     Rental each = (Rental) rentals.nextElement();        Rental each = (Rental) rentals.nextElement();
     result += each.getCharge();                          result += each.getFrequentRenterPoints();
   }                                                    }
   return result;                                       return result;
 }                                                    }



Two very similar loops. Can we factor out the
looping code?


private int accumulate(Accumlator a) {
  Enumeration rentals = _rentals.elements();
   while (rentals.hasMoreElements()) {
        a.accum(rentals.nextElement());
   }
   return result;
 }
                    Definition
• Noun form:
  – A change made to the internal structure of software to
    make it easier to understand and cheaper to modify
    without changing its observable behavior.
• Is refactoring just “cleaning up code”?
  – Kind of.
• Note that changes must be to make the code
  easier to understand and modify.
  – If for performance, it doesn’t count.
• Separate adding functionality from refactoring.
Dubious Code
•   Duplicated code
•   Long method
•   Large class
•   Long parameter list
•   Divergent change: When you change the same class for different
    reasons, such as change a single class when you add a monster,
    then change the same class when you add a weapon type.
•   Shotgun change: When you need to do something routine, such as
    add another monster type, you find yourself making changes to
    multiple places.
•   Feature envy: When a method seems more interested in the data of
    another class than the class it belongs to.
•   Data clumps: Pieces of data that always seem to be together.
•   Primitive obsession: Use of things like an operation code that is an
    int, when it could be an object.
•   Switch statements: Especially on type codes. Replace with
    polymorphism.
•   Parallel inheritance hierarchies: Every time you make a subclass
    in one hierarchy, you have to make another subclass in another
    hierarchy.
• Lazy class: Classes that are not doing much of anything.
• Speculative generality: Too much generality (over-generic, over-
  engineering).
• Temporary field: Member variables that are only set in certain
  circumstances.
• Message chains: Repeated use of “get” methods in a sequence.
• Middle man: Unnecessary delegation.
• Inappropriate intimacy: Classes that make too much use of private
  methods/members.
• Alternative classes with different interfaces: Multiple classes that
  are alternatives, but have different interfaces.
• Incomplete library class: Library class that needs some other
  methods.
• Data class: Class that has operations, but the operations are done
  by clients.
• Refused bequest: Inheritance that is not used.
• Excessive commenting: Indicates that maybe the code is messy.
                    Duplicated Code
• Why is it bad?
   – Often forget to make changes in both places, leading to divergence.
• Why does it exist?
   – Sometimes it is purely duplicated, someone was too lazy to make a
     method.
   – Sometimes it is not completely obvious how to factor the duplication out.
• Fixing:
   – If in same class, then put in method.
   – If in separate classes, then put in function, or base class. (How to
     choose?)
   – What if not quite the same?
       •    Template method
       •    Abstract factory
       •    Builder
       •    Functor
       •    Polymorphism
                  Long Method
•   Printers.




• Why are they bad?
      – Easier to understand (for others).
      – Breaking up forces you to think about it.
      – Promotes re-use.
• Favoring queries over temporaries make it
  easier to extract a method.
• Should the extracted code be a method or
  a function? (What is the difference?)
            Large Class
• Why does this happen?
        Long Parameter List
• Why bad?
  – A pain to use, so prone to error. Was it
    update(row, col), or update(col, row)?
• How to fix?
  – Allow a method to get the data it needs
    without passing it in directly. For example,
    pass in an object. Or put information in
    superclass.
          Divergent Change
• You change different parts of a class,
  depending on what happens in the
  domain.
  – For example, when you add a new database,
    you change class A, when you add a new
    financial transaction type, you also change
    class A, but the parts that you change don’t
    overlap.
• So what?
• Solution is to separate into two classes.
           Shotgun Surgery
• You change one aspect in the domain, but
  have to change multiple places in the
  code.
  – You add a weapon type, but have to change
    multiple places.
• Why does this happen?
  – Catalogs/registries/menus (more innocuous)
    • Can it be solved?
                     Feature Envy
• In a piece of code, you need to execute a bunch of get
  methods on another class.
• Solution is to move the method to the class that it uses
  the most data from.
   – What if it uses data from multiple classes?
• Should you always do this? What is the downside?
   – Contradicts keeping algorithms and data structures separate.
   – What about Strategy and Visitor patterns?
   – Encourage localized change. Things that change together
     should go together.
   – If you want to keep them separate, but want to reduce
     complexity, what should you do?
       • Abstract out a smaller number of operations that the algorithms
         might need.
               Data Clumps
• Pieces of data that are often seen
  together:
  – x, z, and y coord.
     • How do you represent the amount of fog in a
       room?
     • How do you represent the current flow in a room?
  – Name, street, city
  – One test, delete the data values. Does it still
    make sense?
         Primitive Obsession
• Not replacing primitives with objects.
  – Suppose you need to keep track of time. Can
    you use a primitive?
     • What happens when you add current time plus
       time five hours ago?
     • What do you get when you subtract two times?
  – Speed, should you use a class?
  – Unit analysis.
  – Telephone numbers, credit card numbers?
            Switch Statements
• Why are they bad?
  – Duplication
  – Often puts behavior outside of the class.
  – Error prone.
• Can usually replace with:
  –   Polymorphism
  –   Builder
  –   Strategy
  –   Template method
  –   Functor (command)
  –   Template
Parallel Inheritance Hierarchies
• For example, if everytime you created a new
  action in your GUI, you had to create a new
  menu item.
• Why does it happen?
  – Somehow, you have a dependency between two
    classes that is not really part of the domain.
• How to fix?
  – Use Command.
  – Use polymorphism.
  – Move methods/data.
                  Lazy Class
• Classes that just don’t do much.
• How do they happen?
  – Refactoring
  – Changes to initial design.
     • Perhaps an interest calculation was simplified.
       Speculative Generality
• Start a project, imagine every single
  possibility, spend years designing it…
  – Hooks, special cases, etc.
• This is over-engineering.
• On the other hand, you can also under-
  engineer.
• It takes judgment to find the right balance.
                  Temporary Field
• Sometimes you see an object where the instance variable only in
  certain situations. It seems to be unused in other cases.
• When does this happen?
   – Suppose you have a piece of code that calculates some intermediate
     values.
   – You decide to break it up into several pieces.
       • Say you are calculating the effects of a projectile impact on armor for a
         game.
       • First you calculate the parameters of the impact (energy, etc.)
       • Then you want to animate some effects, but using a separate method. There
         are five parameters that are needed in the animate method, that you
         calculated from the impact.
       • You could use member variables to avoid passing parameters.
• Solution?
   – Introduce another object.
                Message Chains
• Follow a chain of links:
   – ch = vehicle->getChassis();
     body = ch->getBody();
     shell = body->getShell();
     material = shell->getMaterial();
     props = material->getProperties();
     color = props->getColor();
• Why is this bad?
   – Client code is dependent on the hierarchy.
   – Tedious.
• Solution?
   – May not be a great one.
      • Use delegation.
      • Try to find out what is actually being done with the data.
               Middle Man
• A class that delegates for no reason.
• What are legitimate reasons for
  delegation?
  – Adapter/proxy/bridge/decorator
  – Composition
• Is delegation better or inheritance better
  for extending functionality?
       Inappropriate Intimacy
• Classes that are too interdependent.
• Why does this happen?
  – Initial separation not well designed, need this,
    need that.
• Separate, move methods, move fields.
• Change associations.
 Alternative Classes with Different
             Interfaces
• Class A and class B essentially do the same
  thing, but have different interfaces. (Or have two
  methods that essentially do the same thing.)
• Use abstraction and inheritance.
• Can also happen with two methods that
  essentially do the same thing.
  – Semantically same kind of operation, but on different
    data?
  – Different kind of operation, but on same data.
      Incomplete Library Class
• A library class that doesn’t quite do enough.
  – So you have code that is in the client:
     • LibClass obj;
       // Do an op on obj, but essentially externally.
       obj->getData1();
       // …
       obj->getData2();
       // …
• Solution?
  – For something simple, curse and do nothing.
  – Used twice, and a little bit complicated, use a static
    method in the client class. Make clear that it should
    be in the library class.
  – More complex? Use a wrapper or subclass.
              Data Class
• This is a class that has no methods.
• See where it is being used. Move the code
  into the class as a method.
• Often happens early in design, so not
  necessarily an indication of bad design,
  just incomplete.
            Refused Bequest
• A subclass doesn’t use either data or code from
  superclass. A superclass should only have what
  is in common. If it is not used by a subclass,
  then it is not in common.
  – One option. Determine what is not used. Push it down
    to a sibling class.
  – Say you have Cartesian and polar coordinates. The
    angle and length are in the base class. A derived
    cartesian class does not use those.
  – Say you have an projectile class with a explosion
    method. The base class one does a simple one. You
    derive a class that has a more cool looking one.
         Excessive Commenting
• Are these comments useful?
   – // Add 1 to x and store in y.
     y = x + 1;
     // Swap the values of the 2nd and 1st bit.
     x = (x&~3) | (x&1 << 1) | (x&2 >> 1);
     // Fire all weapons.
     for (weapon_iter = armament->begin(); weapon_iter.done();
     weapon_iter++) {
       weapon_iter->fire();
     }
• Comments should not be used as a substitute for clear
  coding.
• Too many long comments promotes wrong comments,
  where the code does not match the comments.
   – // Add 1 to x and store in y.
     y = x + 2;
                 State Space
• Closed systems can be in states.
  – Are there a finite number of states, or infinite number?
• How do you describe the state?
  – Each parameter can be an axis.
  – A state is then a point. Are all points valid?
  – Suppose we plot a point, then the next instant in time
    we plot another point.
  – The state through time is a trajectory. What is length
    along the trajectory? Can trajectories cross?
• In terms of trajectories, what does a correct
  system look like?
                     Testing
• What kinds of testing are there?
  – Unit, integrative, system
  – White box, gray box
  – Advantages/disadvantages?
• Which is longer, the thing being tested, or the
  test code?
• How do you test a hash table, or a red-black
  tree?
  – Code coverage
  – Does code coverage cut it for this?
  – You really need to have a complete search of the
    state space.
• What is a race condition?
• How do you test concurrent code?
  – Say you have a thread-safe hash table.
• Ways of doing tests.
  – Each class has a test class(es).
  – Each class has a test method.
  – Full test harness.
          Model Checking
• Suppose you have a data structure with
  one method with two parameters which
  may range in value from 1-3.
• Suppose the data structure has two
  integer variables, which may range in
  value from 1-10.
• What is the state space?
• Can completely prove that this data
  structure is correct?

				
DOCUMENT INFO