6
\$\begingroup\$

I am learning java and I successfully wrote one small console application. I would love to get reviews and possible bug sources on my code. I'm particularly concerned with my object structure.

Question

A well renowned hotel has three branches in Miami. Namely x,y and z. Each has two types of customers: Regular and Rewardee. Also each branch has its own ratings x is given a 3 star rating while y has 5 star rating and z has 4 star rating.

Each hotel has specific rates for weekend and weekdays. x charges $100 for regular customers on weekdays and $120 on weekends While it is $90 for rewardee on weekdays and $60 on weekends. Similarly y charges $130 for regular customers on weekdays and $150 on weekends. While its $100 for rewardee on weekdays and $95 on weekends. While z charges $195 for regular customers on weekdays and $150 on weekends. While its $120 for rewardee on weekdays and $90 on weekends. Now when the customer requests for a particular detail you need to find which hotel would yield the customer profit. In case of tie between hotels compare the ratings and provide the result.

Input format:

regular: 16Mar2010(sun), 19Mar2010(wed), 21Mar2010(Fri)

Output format:

320
410
540
LakeWood

Solution:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;

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(int regularWeekEnd) {
        this.regularWeekEnd = regularWeekEnd;
    }

    public int getRewardeeWeekDay() {
        return rewardeeWeekDay;
    }

    public void setRewardeeWeekDay(int rewardeeWeekDay) {
        this.rewardeeWeekDay = rewardeeWeekDay;
    }

    public int getRewardeeWeekEnd() {
        return rewardeeWeekEnd;
    }

    public void setRewardeeWeekEnd(int rewardeeWeekEnd) {
        this.rewardeeWeekEnd = rewardeeWeekEnd;
    }

    public String getHotelName() {
        return hotelName;
    }
}

public class TestHotel {

    private static HotelFactory x, y, z;

    public static void main(String[] args) throws IOException {
        BufferedReader bf = new BufferedReader(new InputStreamReader(System.in));
        String s = bf.readLine();

        x = new HotelFactory("LakeWood");
        x.setRegularWeekDay(100);
        x.setRegularWeekEnd(120);
        x.setRewardeeWeekDay(90);
        x.setRewardeeWeekEnd(60);

        y = new HotelFactory("RidgeWood");
        y.setRegularWeekDay(130);
        y.setRegularWeekEnd(150);
        y.setRewardeeWeekDay(100);
        y.setRewardeeWeekEnd(95);

        z = new HotelFactory("BridgeWood");
        z.setRegularWeekDay(195);
        z.setRegularWeekEnd(150);
        z.setRewardeeWeekDay(120);
        z.setRewardeeWeekEnd(90);

        int index = s.indexOf(":");
        String type = s.substring(0, index);

        int cost_x = 0, cost_y = 0, cost_z = 0;
        int day_index_start = 0, day_index_end = 0;

        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();
                    }
                }
            }
        }
        System.out.println(cost_x);
        System.out.println(cost_y);
        System.out.println(cost_z);

        String result = min(cost_x, cost_y, cost_z);
        System.out.println(result);
    }

    private static String min(int a, int b, int c) {
        if (a < b && a < c) {
            return x.getHotelName();
        } else if (b < a && b < c) {
            return y.getHotelName();
        } else if (c < a && c < b) {
            return z.getHotelName();
        } else if (a == b || b == c) {
            return y.getHotelName();
        } else if (c == a) {
            return z.getHotelName();
        } else {
            return x.getHotelName();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Hi! Welcome to Code Review! Please try to use question titles that describe what your code does. I've edited it, but please feel free to change it if you feel it could be better. \$\endgroup\$ – RubberDuck Sep 8 '14 at 10:59
13
\$\begingroup\$

There are a couple of issue with your code:

  • HotelFactory is not a factory, it's a domain object. It should be called simply "Hotel"
  • Never ever name your variables or methods x, y and z. Never ever name your parameters a, b and c. You should specifically never ever combine these two.
    There's a special hell for people who do that :D
  • Your Hotels have properties that would not change, correct? Then your Hotel objects should be immutable, meaning there are no setter methods for those properties, which should then be initialized via the constructor
static class HotelFactory {
    private final String hotelName;
    private final int regularWeekDay;
    private final int regularWeekEnd;
    private final int rewardeeWeekDay;
    private final int rewardeeWeekEnd;

    public HotelFactory(String hotelName, int regularWeekDay, int regularWeekEnd, int rewardeeWeekDay, int rewardeeWeekEnd) {
        this.hotelName = hotelName;
        this.regularWeekDay = regularWeekDay;
        this.regularWeekEnd = regularWeekEnd;
        this.rewardeeWeekDay = rewardeeWeekDay;
        this.rewardeeWeekEnd = rewardeeWeekEnd;
    }

    public String getHotelName() {
        return hotelName;
    }

    public int getRegularWeekDay() {
        return regularWeekDay;
    }

    public int getRegularWeekEnd() {
        return regularWeekEnd;
    }

    public int getRewardeeWeekDay() {
        return rewardeeWeekDay;
    }

    public int getRewardeeWeekEnd() {
        return rewardeeWeekEnd;
    }
}
  • The min function doesn't actually do a min operation. It does a minPlusSomStuffs operation. That would be related to breaking ties. You should add a javadoc comment describing how the priority order works with ties. There is a Java built-in min function Math.min(), meaning you don't have to manually perform these if checks. But better yet, you can let Java do the sorting for you using a TreeMap:
TreeMap<Integer, HotelFactory> sortedMap = new TreeMap<Integer, HotelFactory>();
sortedMap.put(a, x);
sortedMap.put(b, y);
sortedMap.put(c, z);
return sortedMap.firstEntry().getValue();
  • What if more hotels are added, say 10? What about 100 more? 1000? Your algorithm doesn't scale well, meaning you have to duplicate a lot of code for every hotel you would add. You should think of an min() algorithm that takes a list of hotels. For totaling days based on weekdays and weekends, regular and non-regular, you are duplicating that logic.

    Ask yourself, what if there are more types of customers? More types of days in a weekend? Festival days perhaps? While duplicating code would certainly work, it becomes a maintenance nightmare. Consider a different data-structure that allows for easy lookups based on a combination of keys (the keys being customer type and day type).

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

Here are my suggestions:

The HotelFactory class should be a public class, and therefore in its own file. Also it should be called Hotel instead since it's not doing any factory things.
A factory usually means something that goes through a complex process to create objects, what you are doing is standard simple object creation.

Your HotelFactory class should have a constructor that takes all the parameters at once. Since every time you construct one you set the same values, you should do it all at the same time. This will also potentially reduce or remove the need to have setters for all of the fields.

The min() method should be reworked so that each hotel's name is only in one line.

While your use of the input manipulation works, you might want to explore using the Scanner class as it is very powerful and much more readable.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.