2
\$\begingroup\$

The application I would like help with is a booking calculation tool that runs a few simple calculations in order to work out the cost of someone's stay.

It currently contains only two java files, Booking.java and BookingCalc.java (which is imported into the project from another library). The tool takes both user input (I/O) and runs from file if one is given.

On build (without a file argument passed), the tool asks the user for a series of ints (Type, Price, Guests, Amenities and Days). These then get passed to two methods which calculate the initial costs and apply discount if applicable. The output of the total cost is then rendered to the screen. The build which takes a file as input works in the exact same way other than values from file get parsed before being able to run through the calculation methods.

My question is to ask whether there is a better way to design/refactor this tool. I have read about using the Decorator pattern for I/O but not really sure where to begin. I'm sure there are tons of improvements that could be made to refactor this code. I am after some ideas of how this could be improved/easier to read and build on going forward.

Booking.java

package com.example.booking;

import java.io.File;
import java.util.Scanner;

import java.util.ArrayList;
import java.text.DecimalFormat;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Set;
import java.util.Iterator;
import java.util.regex.*;

public class Booking {
    public static void main(String[] args) throws Exception {

        if (args.length == 1) {

            String fileName = args[0];
            String fileContent = new Scanner(new File(fileName))
                    .useDelimiter("\\Z").next();

            ArrayList<Integer> parsedContent = parseContentFromFileContent(fileContent);

            LinkedHashMap<String, Integer> outputMethod = renderOutput(parsedContent);

            double discountValue = 0.05;

            double subTotal = BookingCalc.calculate(outputMethod.get("Type"), outputMethod.get("Price"), outputMethod.get("Guests"),
                    outputMethod.get("Amenities"), outputMethod.get("Days"));

            System.out.println("Sub-total: £" + new DecimalFormat("#.00").format(subTotal));

            double total = checkForDiscount(outputMethod.get("Days"), subTotal, discountValue);

            System.out.println("Total: £" + new DecimalFormat("#.00").format(total));
            return;
        }

        Scanner scan = new Scanner(System.in);

        System.out.println("Enter your type of booking:");
        int type = scan.nextInt();

        System.out.println("Enter the standard rate");
        int price = scan.nextInt();

        System.out.println("Enter the number of guests");
        int guests = scan.nextInt();

        System.out.println("Enter costs for any other amenities");
        int amenities = scan.nextInt();

        System.out.println("Enter the number of days");
        int days = scan.nextInt();

        double discountValue = 0.05;
        double subTotal = BookingCalc.calculate(type, price, guests, amenities, days);

        System.out.println("Sub-total: £" + new DecimalFormat("#.00").format(subTotal));

        double total = checkForDiscount(days, subTotal, discountValue);

        System.out.println("Total: £" + new DecimalFormat("#.00").format(total));
    }

    public static double checkForDiscount(int days, double subTotal, double discountValue) {
        double newPrice = subTotal;

        if (days >= 14) {
            double calculation = subTotal * discountValue;
            System.out.println("Discount: £" + calculation);
            newPrice = applyDiscount(discountValue, subTotal);
        } else {
            System.out.println("No discount");
        }
        return newPrice;
    }

    public static double applyDiscount(double discountValue, double subTotal) {
        return subTotal - (subTotal * discountValue);
    }

    public static ArrayList<Integer> parseContentFromFileContent(String fileContent) {

        ArrayList<Integer> parsedInts = new ArrayList<>();

        String pattern = "([a-z]{2})-(\\d+)\\n[g]-(\\d+)\\n([a-z]{4})-(\\d+)";
        Pattern p = Pattern.compile(pattern, Pattern.DOTALL);
        Matcher m = p.matcher(fileContent);

        if (m.matches()) {
            switch (m.group(1)) {
                case "sr":
                    parsedInts.add(1);
                    break;
                case "pr":
                    parsedInts.add(2);
                    break;
                case "eh":
                    parsedInts.add(3);
                    break;
            }

            parsedInts.add(Integer.parseInt(m.group(2)));
            parsedInts.add(Integer.parseInt(m.group(3)));

            switch (m.group(4)) {
                case "kitc":
                    parsedInts.add(1);
                    break;
                case "park":
                    parsedInts.add(2);
                    break;
                case "wifi":
                    parsedInts.add(3);
                    break;
            }
            parsedInts.add(Integer.parseInt(m.group(5)));
        }
        return parsedInts;
    }

    public static LinkedHashMap<String, Integer> renderOutput(ArrayList<Integer> parsedContent) {
        LinkedHashMap<String, Integer> renderedOutputMap = new LinkedHashMap<String, Integer>();

        String type = "Type";
        String price = "Price";
        String guests = "Guests";
        String amenities = "Amenities";
        String days = "Days";

        renderedOutputMap.put(type, parsedContent.get(0));
        renderedOutputMap.put(price, parsedContent.get(1));
        renderedOutputMap.put(guests, parsedContent.get(2));
        renderedOutputMap.put(amenities, parsedContent.get(3));
        renderedOutputMap.put(days, parsedContent.get(4));

        Set set = renderedOutputMap.entrySet();
        Iterator iterator = set.iterator();
        while(iterator.hasNext()) {
            Map.Entry mentry = (Map.Entry)iterator.next();
            System.out.println(mentry.getKey() + ": " + mentry.getValue());
        }

        return renderedOutputMap;
    }

}

BookingCalc.java

package com.example.booking;

public class BookingCalc {
    public BookingCalc() {
    }

    public static double calculate(int var0, int var1, int var2, int var3, int var4) {
        if(var2 <= 1000000) {
            int baseCost = (var0 * var1);
            int timesDays = (baseCost * var4);
            int extras = timesDays + var2 + var3;
            return extras;
        } else {
            throw new RuntimeException("Invalid calculation");
        }
    }
}

Sample input from file for Type, Price, Guests, Amenities, Days:

sr-25   /* Groups 1 & 2 (Two letters and a number) */
g-4     /* Group 3 (A number) */
kitc-10 /* Groups 4 & 5 (A letter and a number) */

eh-46
g-2
wifi-16
\$\endgroup\$
  • \$\begingroup\$ The calculation formula is truly bizarre: it both multiplies by var4 and adds var4, which seems to be the number of days. The unit analysis does not make sense. Furthermore, the pattern has ([a-z]{4}) for group 4, which could only match "wifi". Putting the question back on hold. \$\endgroup\$ – 200_success Feb 27 '17 at 19:05
  • \$\begingroup\$ @200_success Yes, sorry the calculation was rushed. I have updated that now and the regex to match a more accurate input. I have also added this code to GitHub as it will be easier to make changes going forward. But I'm not sure if I'm allowed to post links to external sites on here? github.com/no-direction/bookingCalculator \$\endgroup\$ – NoDirection Feb 27 '17 at 20:02
1
\$\begingroup\$

Decorator

The Decorator pattern allows you to mix and match functionality that you want to add. It's not immediately obvious to me that that is what you want to do. So I'm not going to try to fit this into a Decorator pattern. Perhaps you might consider the Adapter pattern (files and standard input) instead. Or the Facade pattern for your calculation.

If you want to make a new version that mixes and matches things, you could post a new question that already does that and someone could help you make it better. I'll focus on changes I'd make to the existing code.

Don't duplicate code

Your original code runs separate calculations and display for each version of the input. But both versions do the calculations and display the same way. Consider

    public static void main(String[] args) throws Exception {

        int type, price, guests, amenities, days;
        if (args.length == 1) {
            try (Scanner scan = new Scanner(new File(args[0]))) {
                String fileContent = scan.useDelimiter("\\Z").next();

                List<Integer> parsedContent = parseContentFromFileContent(fileContent);

                Map<String, Integer> outputMethod = renderOutput(parsedContent);

                type = outputMethod.get("Type");
                price = outputMethod.get("Price");
                guests = outputMethod.get("Guests");
                amenities = outputMethod.get("Amenities");
                days = outputMethod.get("Days");
            }
        } else {
            try (Scanner scan = new Scanner(System.in)) {
                System.out.println("Enter your type of booking:");
                type = scan.nextInt();

                System.out.println("Enter the standard rate");
                price = scan.nextInt();

                System.out.println("Enter the number of guests");
                guests = scan.nextInt();

                System.out.println("Enter costs for any other amenities");
                amenities = scan.nextInt();

                System.out.println("Enter the number of days");
                days = scan.nextInt();
            }
        }

        double discountValue = 0.05;
        double subTotal = BookingCalc.calculate(type, price, guests, amenities, days);
        System.out.println("Sub-total: £" + new DecimalFormat("#.00").format(subTotal));

        double total = checkForDiscount(days, subTotal, discountValue);
        System.out.println("Total: £" + new DecimalFormat("#.00").format(total));
    }

By putting the input in the same variables either way, we have only one set of calculations and output. So if we need to change it, we only need to do so in one place. Even better would be changing the input for BookingCalc.calculate to take a different data structure (e.g. a Map with the same keys as produced by renderOutput). But perhaps you can't access that.

I also added try-with-resources blocks to manage the Scanner resources. This got rid of a compiler warning for me.

I changed the types of parsedContent and outputMethod to the interfaces. Now we can change the implementation in just one place. Everywhere else it's the same regardless of implementation.

Prefer for to an Iterator

        Set set = renderedOutputMap.entrySet();
        Iterator iterator = set.iterator();
        while(iterator.hasNext()) {
            Map.Entry mentry = (Map.Entry)iterator.next();
            System.out.println(mentry.getKey() + ": " + mentry.getValue());
        }

This seems unnecessarily complicated to me. Consider

        for (Map.Entry<String, Integer> mentry : renderedOutputMap.entrySet()) {
            System.out.println(mentry.getKey() + ": " + mentry.getValue());
        }

This drops six lines down to three, two of which are the same. And it gets rid of manually handling the iterator. This way, the compiler worries about the fiddly bits.

There are times when an Iterator is necessary. This isn't one of them.

\$\endgroup\$
1
\$\begingroup\$

Your calculation still seems suspect: you're adding numbers that don't seem like they should be amounts of money. Nevertheless, I'll play along.

Your main problem is that your model is weak. ArrayList<Integer> and HashMap<String, Integer> are both poor ways to represent a booking.

In fact, I would say that you have named your classes backwards. The "calculator" is the application; the "booking" should be the model.

The main challenge for you is to accept a booking from either a file or the console. For that, you want a Booking.Source interface that has a readBooking() method. Then, you can instantiate either a BookingFileReader or a BookingPrompter, depending on which strategy is needed.

In the code below, I'm being sloppy with error handling, but the main point should be clear enough.

Booking.java

Most of the business logic lives in this model. I've used enums for the string-to-int translations.

import java.io.IOException;
import java.text.DecimalFormat;

public class Booking {
    public interface Source {
        boolean hasNextBooking();
        Booking readBooking() throws IOException;
    }

    public static enum Type {
        SR(1),
        PR(2),
        EH(3);

        public final int multiple;

        Type(int multiple) {
            this.multiple = multiple;
        }

        public static Type forMultiple(int multiple) {
            for (Type t : values()) {
                if (t.multiple == multiple) return t;
            }
            return null;
        }
    }

    public static enum Amenity {
        KITC(1),
        PARK(2),
        WIFI(3);

        public final int fee;

        Amenity(int fee) {
            this.fee = fee;
        }

        public static Amenity forFee(int fee) {
            for (Amenity a : values()) {
                if (a.fee == fee) return a;
            }
            return null;
        }
    }


    private Booking.Type type;
    private Booking.Amenity amenity;
    private int dailyRate, guestCount, days;

    public void setType(Booking.Type t) {
        this.type = t;
    }

    public void setDailyRate(int r) {
        this.dailyRate = r;
    }

    public void setGuestCount(int g) {
        this.guestCount = g;
    }

    public void setAmenity(Amenity a) {
        this.amenity = a;
    }

    public void setDays(int d) {
        this.days = d;
    }

    public double subtotal() {
        return (this.type.multiple * this.dailyRate * this.days)
             + this.guestCount
             + this.amenity.fee;
    }

    public double discount() {
        return (this.days >= 14) ? 0.05 * this.subtotal() : 0.0;
    }

    public double total() {
        return this.subtotal() - this.discount();
    }

    public String toString() {
        DecimalFormat fmt = new DecimalFormat("#.00");

        StringBuilder s = new StringBuilder();
        s.append(  "Type: ").append(this.type.multiple)
         .append("\nPrice: ").append(this.dailyRate)
         .append("\nGuests: ").append(this.guestCount)
         .append("\nAmenities: ").append(this.amenity.fee)
         .append("\nDays: ").append(this.days);
        if (this.discount() == 0.0) {
            s.append("\nNo discount");
        } else {
            s.append("\nDiscount: £").append(fmt.format(this.discount()));
        }
        s.append("\nTotal: £").append(fmt.format(this.total()));
        return s.toString();
    }
}

BookingFileReader.java

I've used named capture groups to avoid counting parentheses.

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.NoSuchElementException;
import java.util.Scanner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class BookingFileReader implements Booking.Source {
    private static final Pattern REGEX = Pattern.compile(
        "(?<Type>[a-z]{2})-(?<DailyRate>\\d+)\\n" +
        "g-(?<GuestCount>\\d+)\\n" +
        "(?<Amenity>[a-z]{4})-(?<Days>\\d+)\\s*",
        Pattern.DOTALL
    );

    private final Scanner scanner;

    public BookingFileReader(File f) throws FileNotFoundException {
        this.scanner = new Scanner(f);
        this.scanner.useDelimiter("\\Z|\n\n");
    }

    public boolean hasNextBooking() {
        return this.scanner.hasNext(REGEX);
    }

    public Booking readBooking() throws IOException {
        try {
            String s = this.scanner.next(REGEX);
            Matcher m = REGEX.matcher(s);
            m.matches();

            Booking b = new Booking();
            b.setType(Booking.Type.valueOf(m.group("Type").toUpperCase()));
            b.setDailyRate(Integer.valueOf(m.group("DailyRate")));
            b.setGuestCount(Integer.valueOf(m.group("GuestCount")));
            b.setAmenity(Booking.Amenity.valueOf(m.group("Amenity").toUpperCase()));
            b.setDays(Integer.valueOf(m.group("Days")));
            return b;
        } catch (NoSuchElementException nothing) {
            if (this.scanner.ioException() != null) throw this.scanner.ioException();
            return null;
        }
    }
}

BookingPrompter.java

import java.io.IOException;
import java.util.Scanner;

public class BookingPrompter implements Booking.Source {
    Scanner scanner = new Scanner(System.in);

    public boolean hasNextBooking() {
        return this.scanner != null;
    }

    public Booking readBooking() throws IOException {
        Booking b = new Booking();
        b.setType(Booking.Type.forMultiple(askInt("Enter your type of booking: ")));
        b.setDailyRate(askInt("Enter the standard rate: "));
        b.setGuestCount(askInt("Enter the number of guests: "));
        b.setAmenity(Booking.Amenity.forFee(askInt("Enter costs for any other amenities: ")));
        b.setDays(askInt("Enter the number of days: "));

        this.scanner.close();
        this.scanner = null;

        return b;
    }

    private int askInt(String prompt) throws IOException {
        System.out.print(prompt);
        if (this.scanner.hasNextInt()) {
            return this.scanner.nextInt();
        }
        throw this.scanner.ioException();
    }
}

BookingCalc.java

Behold, there is hardly any code here at all!

import java.io.File;
import java.io.IOException;

public class BookingCalc {
    public static void main(String[] args) throws IOException {
        Booking.Source src = (args.length == 1) ? new BookingFileReader(new File(args[0]))
                                                : new BookingPrompter();
        while (src.hasNextBooking()) {
            Booking booking = src.readBooking();
            System.out.println(booking);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Those changes seem to be on the right lines. I can assure you the data needed for the input is now correct. Type of room may seem odd to be converted to an int and used in the calculation but it does matter for our scenario. The only issue I have is that BookingCalc is some legacy code and is soon to be updated and something I can't change. Sorry if this wasn't clear in description. But I like the fact Booking.java now contains a bunch of setters and that two key pieces of functionality is now split into BookingFileReader.java and BookingPrompter.java. \$\endgroup\$ – NoDirection Feb 28 '17 at 9:19

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.