The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

Ugly Code-How to refactor

This function gives me total rate of rooms to be hired for an itinerary for a person.I am sure its a bad code.How can I improve on it?

 
public TotalRoomRatesItinerary getTotalHotelRates()
        {
            TotalRoomRatesItinerary RoomRates = new TotalRoomRatesItinerary();
            foreach (ItineraryDay Day in m_ItineraryDay)
            {
                foreach (ItineraryHotelInformation Hotel in Day.getHotels())
                {
                    foreach (ItineraryHotelRoomType RoomType in Hotel.m_RoomTypeList)
                    {
                        foreach (ItineraryRoomRate Rate in RoomType.m_RoomRate)
                        {
                            if (Rate.m_RoomRate.BedCategory.BedCapacity == 1)
                            {
                                RoomRates.SR += Rate.m_RoomRate.Rate;
                            }
                            else if (Rate.m_RoomRate.BedCategory.BedCapacity == 2)
                            {
                                RoomRates.DR += Rate.m_RoomRate.Rate;
                            }

                            else if (Rate.m_RoomRate.BedCategory.BedCapacity == 3)
                            {
                                RoomRates.TR += Rate.m_RoomRate.Rate;
                            }
                        }
                    }
                }
            }

            return RoomRates;
        }
jam
Sunday, March 11, 2007
 
 
The only thing "wrong" with your code is that the nested loops make it hard to read.

Consider adding a "GetRates()" method to your ItineraryDay, ItineraryHotelInformation, and ItineraryRoomRate classes.

Then, Day.GetRates() would return to sum of GetRates() from Hotels, which would return the sum of GetRates() from the RoomRate class, which would wrap the decision tree on which rate to charge.
Chris McKenzie Send private email
Monday, March 12, 2007
 
 
I see a couple of other things wrong with it:

Rate.m_RoomRate.BedCategory.BedCapacity is a red flag.  Well, it's *three* red flags:  it's a sign that the law of Demeter's being disregarded, that an object is modifying another object's field directly rather than going through a property accessor, and that someone out there is still using the "m_" prefix.

The if/elseif/else construct is fragile, especially since the same unwieldy expression is being used in each conditional.  (What if you need to change it, and you don't change it in all three places?)  At the least I'd use a switch() there.

Depending on how often you have to iterate through all of this stuff, you might consider building a property in ItineraryDay that exposes an IEnumerable list of Rate objects.
Robert Rossney Send private email
Monday, March 12, 2007
 
 
+1 for making methods in each class in your hierarchy.

Also, get rid of the magic "room-type" numbers == 1, == 2

The m_ prefix appears to indicate that you are accessing member data from outside the object which you shouldn't be (anything accessed from outside should always have a natural name, regardless of whether you also use natural names for private data - this aids in programmer discoverability when browsing the methods - with Intellisense or without)
Cade Roux Send private email
Monday, March 12, 2007
 
 
Where is this data coming from?  Based on "ItineraryHotelInformation Hotel in Day.getHotels()" I'm betting a database.

Your best best is to reactor into SQL.  All this nested loopage is probably creating bunch of uneeded database load that could be rolled up into a couple queries.
Cory R. King
Monday, March 12, 2007
 
 
thks for suggestions,i will work on it and come back with new code.
jam
Tuesday, March 13, 2007
 
 
If I were you,
I will extract the if..else..if and all the loops to single functions (extract method). And then change if..else..if to switch..case.
Koms Bomb Send private email
Tuesday, March 13, 2007
 
 
"Also, get rid of the magic "room-type" numbers == 1, == 2"

I think on closer inspection you will find that these are the numbers of beds in the room and therefore not 'magic' numbers.
DJ Clayworth
Wednesday, March 14, 2007
 
 
Not magic numbers, no, though mighty ugly in that RoomRates needs to have a new property added to it (!) and all of the code updating it must be modified if there's ever a room with four beds in it.
Robert Rossney Send private email
Wednesday, March 14, 2007
 
 
As I understand this code, for each day of the itinerary you can have bookings in many hotels, and in each hotel you can book many rooms of different types. The return data is the sum (separately) of the total spent on single, double and triple rooms.

Is that really what you meant? Because it seems odd for an itinerary for a single person, unless they have an entourage. Even then the return data doesn't tell you much unless it also tells you how many rooms of each type were booked.
DJ Clayworth
Wednesday, March 14, 2007
 
 
And Robert, you are right. RoomRates should contain an array, indexed by number of beds, into which the rates can be added. Ideally the array can be extensible (setting new members to zero when you extend it), but dimensioned to a large number like 20 would probably be sufficient.
DJ Clayworth
Wednesday, March 14, 2007
 
 
In my experience the 'sufficiently large array' is a nuisance. Usually you don't want to make the array TOO large because it wastes memory. So you make it just big enough to never be a problem.

Except that a good proportion of the time you end up being wrong about that size. Your software will work perfectly for hotel rooms, but perhaps one day someone will tell the customer it will work for conference rooms as well - all they need to do is change your 'beds' into seats.

Then you've got to write all the error handling code for when your big enough array isn't big enough. Or maybe you don't write it and hope for the best?

Prefer self-extending arrays to big enough arrays, if you're asking me.
Duncan Sharpe Send private email
Sunday, March 18, 2007
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz