12
\$\begingroup\$

Here's the challenge description:

A parking garage charges a $2.00 minimum fee to park for up to three hours.
The garage charges an additional $0.50 per hour for each hour or part thereof in excess of three hours.
The maximum charge for any given 24 hour period is $10.00.
Assume that no car parks for longer than 24 hours at a time.

Write a Java program that calculates and displays the parking charges for each customer who parked in the garage yesterday.
Input the number of hours parked for each customer. The number of customers can be fixed using a final constant.

The program should display the charge for the current customer and should calculate and display the running total of the day's receipts.
Use a method called calculateCharges to determine the charge for each customer.

Here's my code:

import java.text.DecimalFormat;
import java.util.Scanner;

public class Hwk4B {

    public static void main(String[] args) 
    {

        Scanner keyboard = new Scanner(System.in);
        DecimalFormat formatter = new DecimalFormat("$##.00");

        System.out.print("Enter the number of cars parked yesterday: ");
        final int NUMBER_OF_CARS = keyboard.nextInt();

        double hoursParked=0;
        double currentCost=0;   
        double totalCost=0;

        for(int count = 1; count<=NUMBER_OF_CARS; count++)
        {   
            System.out.print("Enter hours parked for car " + count + ": ");
            hoursParked = keyboard.nextDouble();
            currentCost = calculateCharges(hoursParked); //most recent customer
            totalCost+=currentCost; //Running total
        }

        System.out.println("Most recent customer paid: "   +  formatter.format(currentCost));
        System.out.println("Running total of receipts: " + formatter.format(totalCost));

    }

    public static double calculateCharges (double numHours)
    {

        double garageCost = 0;

        if(numHours<=3)
            garageCost = 2;

        else if(numHours>3 && numHours<=19)
            garageCost = 2.0 + 0.5*(numHours - 3);

        else if (numHours >19)
            garageCost = 10;

        return garageCost;
    }

}
\$\endgroup\$
  • 2
    \$\begingroup\$ Shouldn't your sysouts be inside the for loop? \$\endgroup\$ – c_maker Jul 19 '15 at 19:13
  • \$\begingroup\$ @njzk2 Comments are for seeking clarification or help improve the question. Concrete suggestions, no matter how short, belong in answers. \$\endgroup\$ – 200_success Jul 20 '15 at 3:29
11
\$\begingroup\$

You're inconsistent with how you place your curly braces, {}. The general style for Java is to place opening curly braces, {, on the same line, like this:

public void myMethod() {

}

This goes for loops, classes, if statements, etc.


Even though for one-liner if statements, curly braces aren't necessary, I'd still recommend using them. This means that a statement like this:

if (numHours<=3)
    garageCost = 2;

Would become this:

if (numHours<=3) {
    garageCost = 2;
}

Not using curly braces can lead to some not so great mistakes happening. A famous example of this is the Apple SSL bug. This is what happened:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;  /* MISTAKE! THIS LINE SHOULD NOT BE HERE */

Note the lack of curly braces.


I'm also noticing inconsistencies with spacing. For example, this:

else if (numHours>3 && numHours<=19)

Should be changed to this:

else if (numHours > 3 && numHours <= 19)

Generally, it's best to have whitespace between comparison, boolean, binary, mathematical, and assignment operators. It makes your code much clearer, and cleaner to read.


Finally, rather than using concatenation to format strings, I'd use System.out.printf instead. Here's an example:

System.out.printf("Most recent customer paid: %s\n", formatter.format(currentCost));
System.out.printf("Running total of receipts: %s\n", formatter.format(totalCost));

Here's another example:

System.out.printf("Enter hours parked for car %d: ", count);

If you don't like System.out.printf, you can always use String.format.

\$\endgroup\$
  • \$\begingroup\$ thanks, what does the s symbolize in %s? \$\endgroup\$ – Sam Leavitt Jul 19 '15 at 22:53
  • \$\begingroup\$ @SamLeavitt It's the type, in this case, string. \$\endgroup\$ – Ethan Bierlein Jul 19 '15 at 23:43
  • 1
    \$\begingroup\$ It's not the accepted style for Java... It's a matter of preference and actually the subject of a huge debate in the programming community. I prefer braces the way you put them, but it's not a convention that all Java programmers follow... \$\endgroup\$ – Vedaad Shakib Jul 20 '15 at 8:26
  • \$\begingroup\$ @VedaadShakib That's exactly why I'm fed up with Php, Java, C and almost all languages that are permissive about the basics of writing code. Python: batteries included. \$\endgroup\$ – Olivier Pons Jan 6 '16 at 9:26
13
\$\begingroup\$

Your code should be as similar to the spec as possible so:

else if (numHours >19)
     garageCost = 10;

Should be removed and you should add:

return Math.min(garageCost, 10)

My final version is:

public static double calculateCharges (double numHours)
{
    if (numHours <= 3)
    {
         return 2;
    }
    double garageCost = 2.0 + 0.5 * (numHours - 3);
    return Math.min(garageCost, 10);
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You could even push this to the maximum : first garageCost : garageCost = 2.0 + 0.5 * Math.max( numHours - 3, 0), which could lead to return Math.max( 2.0 + 0.5 * Math.max( numHours - 3, 0), 10), but that's a bit harder to understand \$\endgroup\$ – Loufylouf Jul 19 '15 at 19:13
  • \$\begingroup\$ @Loufylouf I think it would be clearer keeping the intermediate var, but removing an if statement. \$\endgroup\$ – Caridorc Jul 19 '15 at 19:15
  • \$\begingroup\$ Totally, I was just saying that if one-liners are your thing, there's a way to do it. In my opinion, you could also remove the first if and initialize garageCost to 2.0, while keeping the code pretty easy to understand. \$\endgroup\$ – Loufylouf Jul 19 '15 at 19:20
  • 3
    \$\begingroup\$ You can clean it up by using a "calc then cap" approach: fee = 2 + 0.5 * (numHours - 3) then return Math.Max(2, Math.Min(10, fee)) \$\endgroup\$ – parker.sikand Jul 20 '15 at 5:17
  • 1
    \$\begingroup\$ @caridorc Your answer is good and for good reasons but for the sake of nit-picking, I could argue that checking for a case of less than 3 hours and then later making a calculation based on subtracting 3 hours is redundant... it is dealing with the 3 hour issue in two different places. If this were changed to 4 hours, the junior dev on the team might miss both occurrences. \$\endgroup\$ – parker.sikand Jul 20 '15 at 5:23
12
\$\begingroup\$

I believe your program is a bit incorrect.

  1. Your program will print only once at the end and not once for each customer.
  2. Because you use double as a data type, I can input partial hours. For example, if I input 4.56 the cost will be $2.78 but really the challenge is asking you to charge $3 in this case. (You can either only input full hours and use int OR you can round the hours up after the input and still use int as the datatype throughout)

And a couple of other comments.

  1. Do not do extra work that is not asked of you. The challenge said the number of customers can be fixed, but you take it as an input. This is not incorrect but in general and especially in isolated cases like this, it is best to stick with the simplest possible solution that results in less code and better understandability of the code. Down the road you will have to live with the extra code and maintain it. E.g. what if you have to put error handling around the user input. Now the two liner goes to 5-6 lines that can break.
  2. You have many magic numbers. You should use meaningful constants for 2, 3, 10, e.g. static final int COST_FOR_FIRST_HOURS = 2; static final int NUMBER_OF_FIRST_HOURS = 3; static final int MAXIMUM_DAILY_COST = 10;. If you use these, it will be easier to read and maintain the code.
  3. NUMBER_OF_CARS should be numberOfCars if you follow Java naming conventions.
  4. And I agree with others, you should just check if the garageCost is over MAXIMUM_DAILY_COST, and if it is, return MAXIMUM_DAILY_COST. If your numbers ever change, you will have to remember to adjust '19' (which you will more than likely forget).
\$\endgroup\$
  • \$\begingroup\$ I thought that I'm supposed to use all caps for a constant that doesn't change? \$\endgroup\$ – Sam Leavitt Jul 19 '15 at 22:51
  • 1
    \$\begingroup\$ Final variables are not constants. You should use all caps for compile time constants. Compile time constants are values baked into your program and do not change from one run to the next. \$\endgroup\$ – c_maker Jul 20 '15 at 0:05
  • \$\begingroup\$ Overall many good points. Excellent review. Just one thing I disagree with: The assignment said that the number of cars can be fixed, it doesn't say that it has to be fixed. \$\endgroup\$ – Simon Forsberg Jul 21 '15 at 0:19
  • \$\begingroup\$ @SimonAndréForsberg I agree with you. Taking the number of cars from the user does not make the code incorrect. However, I still think it is a bad idea so I moved it to general suggestions and explained it a bit more. \$\endgroup\$ – c_maker Jul 21 '15 at 13:29

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.