Cheapest hotel finder

Please help me make this code more object oriented. I also want to use a few more classes and separate the input processing code to another class.

Question:


A hotel chain operating in Goa wishes to offer room reservation services. They have three hotels in Goa: GreenValley, RedRiver and BlueHills. Each hotel has separate weekday and weekend (Saturday and Sunday) rates. There are special rates for rewards customer as a part of loyalty program.Each hotel has a rating assigned to it.

• GreenValley with a rating of 3 has weekday rates as Rs1100 for regular customer and Rs800 for rewards customer. The weekend rates are 900 for regular customer and 800 for a rewards customer.

• RedRiver with a rating of 4 has weekday rates as Rs1600 for regular customer and Rs1100 for rewards customer. The weekend rates are 600 for regular customer and 500 for a rewards customer.

• BlueHills with a rating of 5 has weekday rates as Rs2200 for regular customer and Rs1000 for rewards customer. The weekend rates are 1500 for regular customer and 400 for a rewards customer.

The input to the program will be a range of dates for a regular or rewards customer. The output should be the cheapest available hotel. In case of a tie, the hotel with highest rating should be returned.

Example to the input format:

Regular:

16Mar2009(mon), 17Mar2009(tues), 18Mar2009(wed)


Output:

GreenValley


HotelFactory.java:

`package org.dipankar.java;

public class HotelFactory {
String hotelName;
private int regularWeekDay;
private int regularWeekEnd;
private int rewardeeWeekDay;
private int rewardeeWeekEnd;

HotelFactory(String name) {
this.hotelName = name;
}

public int getRegularWeekDay() {
return regularWeekDay;
}

public void setRegularWeekDay(int regularWeekDay) {
this.regularWeekDay = regularWeekDay;
}

public int getRegularWeekEnd() {
return regularWeekEnd;
}

public void setRegularWeekEnd(

I know this isn't what you want to hear, but it's the low-hanging fruit and somebody's gotta say it anyway.
Indentation

Whenever you see something like this in code:

}
    }
    }
    }

You know something isn't right. In fact, you know something is terribly confusing and broken.

Before you can make any kind of refactoring toward a more OOP approach, you need to fix these issues. When you're done the above "wall of braces" should look like this:

}
            }
        }
    }

Proper indentation is simple actually. Some tools even automate it for you. Take this while loop there (pipes (|) only to show indentation structure):

while (day_index_start != -1) {
|   day_index_start = s.indexOf("(", day_index_start + 1);
|   day_index_end = s.indexOf(")", day_index_end + 1);
|
|   if (day_index_start != -1) {
|   |   String day = s.substring(day_index_start + 1, day_index_end);
|   |   if (day.equalsIgnoreCase("sun") || day.equalsIgnoreCase("sat")) {
|   |   |   if (type.equalsIgnoreCase("regular")) {
|   |   |   |   cost_x += x.getRegularWeekEnd();
|   |   |   |   cost_y += y.getRegularWeekEnd();
|   |   |   |   cost_z += z.getRegularWeekEnd();
|   |   |   } else {
|   |   |   |   cost_x += x.getRewardeeWeekEnd();
|   |   |   |   cost_y += y.getRewardeeWeekEnd();
|   |   |   |   cost_z += z.getRewardeeWeekEnd();
|   |   |   }
|   |   } else {
|   |   |   if (type.equalsIgnoreCase("regular")) {
|   |   |   |   cost_x += x.getRegularWeekDay();
|   |   |   |   cost_y += y.getRegularWeekDay();
|   |   |   |   cost_z += z.getRegularWeekDay();
|   |   |   } else {
|   |   |   |   cost_x += x.getRewardeeWeekDay();
|   |   |   |   cost_y += y.getRewardeeWeekDay();
|   |   |   |   cost_z += z.getRewardeeWeekDay();
|   |   |   }
|   |   }
|   }
}

Notice how much easier it becomes to identify the code blocks. By lining up the closing brace to the beginning of the matching code block, you instantly enhance your code's readability.

Use meaningful names

And declare them as close as possible to their usage.

private static HotelFactory x, y, z;

"x", "y" and "z" are horrible names by any standard. But the worst offender is HotelFactory, because it's lying. Any class named FoobarFactory can be reasonably expected to be a class responsible for creating instances of a Foobar class. So when you name something HotelFactory, readers/reviewers/maintainers are going to expect that the responsibility of a HotelFactory is to create instances of some Hotel class.

But it's not. Your HotelFactory is the Hotel. Rename it as such. And then split the three declarations and move them closer to their usage:

private static Hotel greenValley;
greenValley = new Hotel("Green Valley");
greenValley.setRegularWeekDay(1100);
greenValley.setRegularWeekEnd(900);
greenValley.setRewardeeWeekDay(800);
greenValley.setRewardeeWeekEnd(800);

private static Hotel blueHills;
blueHills = new Hotel("Blue Hills");
//...

Now, you gave that Hotel/HotelFactory class a constructor parameter for the name, but not for the regular/rewardee weekday/weekend prices. Why? If having 5 constructor parameters is a smell, then make another class, and have Hotel take 2 constructor parameters: a string name and a PricingStructure pricing, where PricingStructure could be a simple class exposing the needed members - that way you're not bloating a Hotel's constructor, and you're decoupling a Hotel from the pricing elements.

OOP

What you really want to know, is this:

Given a date and whether we're a "rewardee" customer, what price would we be charging?

So the PricingStructure described above should really be a PricingStrategy, and the interface of that could look like this:

int calculatePrice(Date checkinDate, bool isRewardeeCustomer);

You would have 3 different implementations, one for each hotel, and each hotel's actual pricing structure would become an implementation detail - the client code couldn't care less about how you're going to come up with a price: it only needs to be given an amount, given a date and a bool flag.

And then, if BlueHills wants to offer a 20% special discount on July 4th, that detail is encapsulated in the calculatePrice method, and no other code needs to change anywhere.

In other words, the outside world (i.e. the calling code) only needs to see this:

Hotel greenValley = new Hotel("Green Valley", new GreenValleyPricingStrategy());
Hotel redRiver = new Hotel("Red River", new RedRiverPricingStrategy());
Hotel blueHills = new Hotel("Blue Hills", new BlueHillsPricingStrategy());

And then get them into an array or any iteratable, sort them by whatever their calculatePrice method is returning, and get your answer.

OOP is about scaling: what if the problem statement included a data file that described the pricing strategy for 3,500 hotels? Would min(cost_x, cost_y, cost_z) cut it? You need to look into data structures and possibly sorting

StackExchange Code Review Q#133862, answer score: 23