Source Code: Refactoring Is Still Useful After Doing Use Case-Driven Modeling | Agile Development with ICONIX Process: People, Process, and Pragmatism

Source Code: Refactoring Is Still Useful After Doing Use Case–Driven Modeling

If you check back to the “Figure 8-5, you’ll see the interaction between the MapViewer and the HotelFilter. The MapViewer creates a HotelFilter, and (after getting an AOI and a list of hotels) passes the HotelFilter into the AOI. The AOI then iterates through the list of hotel points (a hotel point is an amenity that we want to filter on), filtering the list using the HotelFilter.

Let’s take a look at the AddHotels method in the AOI class, plus a worker method (Insert) that it delegates to:

 // Get a list of hotels and their properties,  // create a list hotel composite objects  public bool AddHotels (string[] hotelPoints,                         AmenityList Amenities,                         HotelFilter hf)  {      bool SomethingFound = false;      if (hotels == null)          hotels = new ArrayList ();      // hotelPoints are list of      //  0: ID      //  1: X      //  2: Y      //  3: Name      //  4: Address1      //  5: Address2      //  6: City      //  7: State      //  8: Zip      //  9: Phone      // 10: StarRating      // 11: Amenities      // 12: Brand      // 13: PriceBand      // 14: Links      Hotel hp;      long hiBound = hotelPoints.Length;      string hotelAmenities = "";      bool visible;      for (int i=0; i < hiBound; i += 15)      {          try          {              visible = hf.FilterHotel (hotelPoints [i+12],                                        hotelPoints [i+11],                                        ref hotelAmenities);              if (visible)              {                  hp = new Hotel (hotelPoints [i+0],                                  int.Parse (hotelPoints [i+1]),                                  int.Parse (hotelPoints [i+2]),                                  hotelPoints [i+3],                                  hotelPoints [i+4],                                  hotelPoints [i+5],                                  hotelPoints [i+6],                                  hotelPoints [i+7],                                  hotelPoints [i+8],                                  hotelPoints [i+9],                                  hotelPoints [i+10],                                  hotelPoints [i+11],                                  hotelPoints [i+12],                                  hotelPoints [i+13],                                  hotelPoints [i+14]);                  Insert (hp);                  SomethingFound = true;              }          }          catch (Exception ex)          {              string str = ex.Message;              return false;          }      }      return SomethingFound;  }  /// <summary>  /// Insert a HotelOnMap into the hash.  /// If the point is near another point  /// then instead of inserting it into the hash,  /// it inserts the point to the linked list of  /// the HotelPoints that stores these points.  /// </summary>  /// <param name="hp"></param>  private void Insert (Hotel hp)  {      if (hp == null)          return;      HotelComposite hc;      for (int i = 0; i < hotels.Count; i++)      {          hc = (HotelComposite) hotels [i];          if (Math.Abs (hc.GetX () - hp.GetX ()) <= 5 &&              Math.Abs (hc.GetY () - hp.GetY ()) <= 5)          {              // Multiple hotels at the same point              if (hc is HotelCluster)              {                  ((HotelCluster) hc).AddHotel (hp);                  return;              }              else              {                  HotelCluster mhc =                      new HotelCluster (hc.GetX (), hc.GetY ());                  mhc.AddHotel (hc);                  mhc.AddHotel (hp);                  hotels [i] = mhc;                  return;             }         }     }     hotels.Add (hp);  }

We’ve shown hf.FilterHotel in bold because we’ll drill down into the FilterHotel method in just a moment.

While this code is in better shape than the initial state of the code we saw in release 1, there’s still some tidying up that we can do. We’ll cover the finer points of combining refactoring (via unit testing) with ICONIX modeling in Chapter 12. For now, we’ll just cover what needs to be changed and show the modified code.

KNOWING WHEN TO TIDY UP THE CODE

It’s worth emphasizing that the code works just fine in its current state, so what we’re doing here is tidying up to get it ready for the next release. If we knew that this was the final release, or that tidying the code at this stage wouldn’t give us much return on our investment, then it wouldn’t be worth doing. But in this case, the mapplet will (we hope) continue to go through several releases after this book is published, so keeping the code in trim is definitely a good idea.

The first part of this code that’s crying out to be changed is the big list of array indices pointing into the hotelPoints array (where the code is creating a new Hotel object and passing in hotelPoints[i+4], hotelPoints[i+5], etc.). This code is linked to the big code comment just prior to it, which shows what each index in the array means (4 is address1, 5 is address2, etc.).

Whenever you see comments in code, you should immediately pause to think about how the code could be made clearer so that the comments aren’t necessary. In this case, the obvious thing to do is turn the comments into actual constants and use these instead of the numeric literals. This gives us the added benefit of compile-time checking. While we’re at it, here are some more refactorings that the code would benefit from:

We could rename some of the variables so they’re more closely tied in with the domain model (e.g., the variable hiBound could instead be numHotelPoints). Plus, the oddly cryptic variable name hp could instead be visibleHotel, and hf could instead be hotelFilter.
The code contains the numeric literal 15. We could replace this with a constant so that it’s a bit more meaningful (and less error-prone, for example, if the total number of defined hotel points changes), so 15 becomes TOTAL_HOTEL_POINTS (the total number of defined Hotel Points, as distinguished from numHotelPoints in the previous bullet point).
The area of code that creates the visible Hotel could easily be separated into a separate method.

Implementing these refactorings, the code will change as follows:

 // Hotel Points for AddHotels:  internal const TOTAL_HOTEL_POINTS = 15;  internal const int HP_ID = 0;  internal const int HP_X = 1;  internal const int HP_Y = 2;  internal const int HP_NAME = 3;  internal const int HP_ADDRESS1 = 4;  internal const int HP_ADDRESS2 = 5;  internal const int HP_CITY = 6;  internal const int HP_STATE = 7;  internal const int HP_ZIP = 8;  internal const int HP_PHONE = 9;  internal const int HP_STAR_RATING = 10;  internal const int HP_AMENITIES = 11;  internal const int HP_BRAND = 12;  internal const int HP_PRICE_BAND = 13;  internal const int HP_LINKS = 14;  // Get a list of hotels and their properties,  // create a list hotel composite objects  public bool AddHotels (string[] hotelPoints,                         AmenityList Amenities,                         HotelFilter hotelFilter)  {      bool SomethingFound = false;      if (hotels == null)          hotels = new ArrayList ();      Hotel visibleHotel;      long numHotelPoints = hotelPoints.Length;      string hotelAmenities = "";      bool visible;      for (int i=0; i < numHotelPoints; i += TOTAL_HOTEL_POINTS)      {          try          {              visible = hotelFilter.FilterHotel                        (hotelPoints [i+HP_BRAND],                         hotelPoints [i+HP_AMENITIES],                         ref hotelAmenities);              if (visible)              {                  visibleHotel = createVisibleHotel(hotelPoints);                  Insert (visibleHotel);                  SomethingFound = true;              }          }          catch (Exception ex)          {              string str = ex.Message;              return false;          }      }      return SomethingFound;  }  private Hotel createVisibleHotel(string[] hotelPoints)  {      return new Hotel(hotelPoints [i + HP_ID],                       int.Parse (hotelPoints [i + HP_X]),                       int.Parse (hotelPoints [i + HP_Y]),                       hotelPoints [i + HP_NAME],                       hotelPoints [i + HP_ADDRESS1],                       hotelPoints [i + HP_ADDRESS2],                       hotelPoints [i + HP_CITY],                       hotelPoints [i + HP_STATE],                       hotelPoints [i + HP_ZIP],                       hotelPoints [i + HP_PHONE],                       hotelPoints [i + HP_STAR_RATING],                       hotelPoints [i + HP_AMENITIES],                       hotelPoints [i + HP_BRAND],                       hotelPoints [i + HP_PRICE_BAND],                       hotelPoints [i + HP_LINKS]);  }

A really “classy” refactoring (excuse the pun) would be to do away with the way in which the hotelPoints array must be parsed by any code that wants to extract anything from it, and instead encapsulate this behavior in a HotelPoints class. This would tidy up the code immensely, increase its reusability, and so forth. To do this, the first stage would be to move all the HP_ constants (HP_CITY, HP_STATE, etc.) into the new class; make the constants external (so any other class can read them); and create a method called something like CreateHotel(), which would take a string array of hotel point characters. All this really does is move the same code from one class to another. Now that we’ve liberated the code and given it its own class, the next stage would be to make HotelPoints hide away the whole arrays and HP_ constants thing altogether.

Doing this, we’d probably also notice some other areas of the code that could be encapsulated away. We won’t take this particular refactoring any further here, but you get the point.

Tip

Normally, following ICONIX Process will get the code into a nicely factored state, so there shouldn’t be a need to do further refactoring. Sometimes, though, it’s only when you see the code that you realize the design could be tidied up a bit, so in Chapter 12 we describe how ICONIX Process can be used in conjunction with Test-Driven Development (TDD), a code-centric design process.

Next, let’s take a look at the HotelFilter class, which AOI.AddHotels uses to filter out the Hotels that are going to be shown to the user.

 public class HotelFilter  {      private string AmenityFilter;      private string HotelChainFilter;      private string HotelChainName;      private AmenityList Amenities;      private char [] delimiter;      public HotelFilter(AmenityList aAmenities,                         string aAmenityFilter,                         string aHotelChainFilter,                         string aHotelChainName)      {          Amenities = aAmenities;          AmenityFilter = aAmenityFilter;          HotelChainFilter = aHotelChainFilter;          HotelChainName = aHotelChainName;          string delimStr = " ";          delimiter = delimStr.ToCharArray();      }      public string GetFilterText ()      {          if (HotelChainFilter.Length > 0)              return "Currently displaying " + HotelChainName;          if (AmenityFilter.Length > 0)          {              string res = "";              AmenityItem p;              for (int j = 0; j < Amenities.count; j++)              {                  p = Amenities.data [j];                  if (p.abbr != null)                      if (AmenityFilter.IndexOf (p.abbr) >= 0)                      {                          if (res.Length > 120)                          {                              res = res + ", ...";                              break;                          }                          if (res.Length > 0) res = res + ", ";                          res = res + p.val;                      }              }              return "Currently displaying hotels with " + res;          }          return "";      }      public bool FilterHotel (string aHotelChain,                               string Amenity,                               ref string hotelAmenities)      {          string [] sp;          AmenityItem p;          if (HotelChainFilter.Length > 0)              return HotelChainFilter.ToUpper ().                          Equals (aHotelChain.ToUpper ());          else if (AmenityFilter.Length > 0)          {              sp = Amenity.Split (delimiter);              hotelAmenities = "";              for (int j = 0; j < sp.Length; j++)              {                  p = Amenities.Find (sp [j]);                  if (p != null)                      hotelAmenities = hotelAmenities + p.abbr;              }              for (int j = 0; j < AmenityFilter.Length; j++)              {                  if (hotelAmenities.IndexOf                         (AmenityFilter.Substring (j, 1)) < 0)                      return false;              }          }          return true;      }  }

To sum up, it’s a good time to ask if this refactoring is worth it, or if we’re fiddling too much with working code. As we mentioned earlier in the sidebar titled “Knowing When to Tidy Up the Code,” we’re actually preparing the way forward for the next release, so from that perspective it’s worth doing—but in some respects, it’s borderline tinkering.

The ICONIX forward-engineered design got us a pretty clean and fully functional piece of code. But we were still able to find a useful refinement using refactoring principles. If both the following points hold true …

The code is written but you see a way to improve it.
The product (and especially the customer) will benefit from you refactoring the code. That is, there are appreciable business benefits from refactoring the code, such as keeping the design in good shape so the next release will go more smoothly.

… then definitely go for it.