5
\$\begingroup\$

I've created a second 'calculator-like' program, this time using the JOptionPane rather than typing in the console. At the moment the code looks to have a lot of repetition so I'm looking to simplify it; any advice and suggestions would be great!

import javax.swing.JOptionPane;

public class SaveCalc {

    public static void main(String[] args) {

        //Text prompts to collect data
        String carCost = JOptionPane.showInputDialog("What is the total cost of the car?");

        String insCost = JOptionPane.showInputDialog("What is the yearly cost of the insurance?");

        String manCost = JOptionPane.showInputDialog("How much would you like to save for maintenance?");

        String saveMonth = JOptionPane.showInputDialog("How much will you be saving each month?");


        //Converts string inputs to Doubles
        double carCostP = Double.parseDouble(carCost);
        double insCostP = Double.parseDouble(insCost);
        double manCostP = Double.parseDouble(manCost);
        double saveMonthP = Double.parseDouble(saveMonth);

        //Calculates the total cost and the months to save
        double costTotal = carCostP + insCostP + manCostP;
        double months = costTotal / saveMonthP;

        //Converts the months value to an integer
        int monthsP = (int) months;

        System.out.printf
        ("With a saving of \u00A3%5.2f each month and a total cost of \u00A3%5.2f it will take " 
        + monthsP 
        + " months to save up.",saveMonthP,costTotal);
    }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Is there a reason you convert you months from double to int? This would product false results as you would throw away fractions of the last month. Lets say your variable months is of value 4.999 then it would be better to round up. If you're not concerned with less than half a months savings you could use Math.round(), else you may think about using Math.ceil() to always get the full count of months you have to save. \$\endgroup\$
    – Aron_dc
    Commented Sep 21, 2015 at 14:14
  • \$\begingroup\$ I don't think that you have a lot of repetition, personally. Sure, you're repeating similar lines a few times, but for different purposes (asking for and working with different data). You could break the JOptionPane.showInputDialog() plus Double.parseDouble() into a separate function as suggested by TopinFrassi, but the primary benefit of that would be to refocus on the purpose of the particular piece of the code, not really to reduce repetition (because there is so little of that to begin with). \$\endgroup\$
    – user
    Commented Sep 21, 2015 at 18:32
  • \$\begingroup\$ @MichaelKjörling: I think even this small repetition should be put into a function (showing a dialog with a message -> reading the input -> parsing it to double). It would definitely help shorten the code and reduce the level of repetition. It would also shrink this program to about 50% of its original size. He could put his exceptionhandling (NumberFormatException) in there and repeat the reading process as long as it is no valid double value. \$\endgroup\$
    – Aron_dc
    Commented Sep 21, 2015 at 20:15

5 Answers 5

7
\$\begingroup\$

Do not include types in variable names

carCostP
       ^

Remove that P (probably stands for a number type?) the compiler takes care of types for you.

Avoid so many variables

You can avoid those four variables altogether, you may just convert to double as soon as you read the vars, like this:

double carCost = Double.parseDouble(
     JOptionPane.showInputDialog("What is the total cost of the car?"));
\$\endgroup\$
2
  • 2
    \$\begingroup\$ I think the P is there because insCost is already a string variable, so it couldn't be used. I think P stands for "Parsed". Then again, OP will probably answer that! \$\endgroup\$
    – IEatBagels
    Commented Sep 21, 2015 at 14:14
  • \$\begingroup\$ Exactly as you suspected, but using Caridorc's suggestion the need for these 'P' variables has been mitigated. \$\endgroup\$ Commented Sep 21, 2015 at 14:24
3
\$\begingroup\$

Additionally to Caridorcs answer you should avoid using variables for the only reason than changing the type of a result (like you do it with months).

Just cast it to an other type, or better round your months variable, when you need it.

\$\endgroup\$
3
\$\begingroup\$

You don't check if the user's input can be parsed to double, which will cause an exception when you parse. To check if your input is numerical, you can use the following function, which uses a regex :

public static boolean isNumeric(String str)
{
  return str.matches("-?\\d+(\\.\\d+)?");  //match a number with optional '-' and decimal.
}

This example comes from this SO's answer

This way, you can validate your input instead of letting the application crash if I enter non-numerical characters as an input (note that this regex will accept "." and "-" as valid characters).

You need to loop on your input until it is valid, so you could do something like this :

String carCost = JOptionPane.showInputDialog("What is the total cost of the car?");
while(!isNumeric(carCost)){
    carCost = JOptionPane.showInputDialog("Please enter a valid car cost : ");
}

Afterwards, you could extract this logic into a method, let's call it getDoubleInput(String paneText, String invalidInputText)

private String getDoubleInput(String paneText, String invalidInputText){

    String input = JOptionPane.showInputDialog(paneText);
    while(!isNumeric(input)){
        input = JOptionPane.showInputDialog(invalidInputText);
    }

    return input;
}

Then you could, as @Caridorc's pointed, skip the String variable in your code and use this format :

double carCost = Double.parseDouble(getDoubleInput("What is the total cost of the car?","Please enter a valid car cost : "));
double insCostP = Double.parseDouble(getDoubleInput("What is the yearly cost of the insurance?", "Please enter a valid yearly cost of insurance : "));
//...

If you dislike have 2 String parameter to the method, you could use a generic invalid input message such as : "Please enter a valid input : ". Then the method would look like this :

private String getDoubleInput(String paneText){

    String input = JOptionPane.showInputDialog(paneText);
    while(!isNumeric(input)){
        input = JOptionPane.showInputDialog("Please enter a valid input : ");
    }

    return input;
}

You wouldn't need the second parameter in the method call :

double carCost = Double.parseDouble(getDoubleInput("What is the total cost of the car?"));
\$\endgroup\$
3
  • 1
    \$\begingroup\$ You can eliminate the double coded calls to JOptionPane.showInputDialog() by converting the while loop into a do loop. This may or may not increase readability for any one coder in particular; it's a bit of a stylistic choice. Myself, I glossed over the first showInputDialog() call initially, then wondered why you never did anything with paneText, then saw it on the second reading. (Also, there is nothing in the repeat prompt to indicate to the user what data to enter. If you're feeling fancy, the conditional operator ?: can be put to good use here.) \$\endgroup\$
    – user
    Commented Sep 21, 2015 at 18:35
  • \$\begingroup\$ I felt like repeating it was better than using the do/while + ?:! :) \$\endgroup\$
    – IEatBagels
    Commented Sep 21, 2015 at 18:43
  • 1
    \$\begingroup\$ Yes, like I said, it's a bit of a stylistic choice. Functionally it's the same. \$\endgroup\$
    – user
    Commented Sep 21, 2015 at 18:44
2
\$\begingroup\$

A small but possibly important point (or so I think ;)):

.. this time using the JOptionPane rather than typing in the console...

And then you have this:

System.out.printf
("With a saving of \u00A3%5.2f each month and a total cost of \u00A3%5.2f it will take " 
+ monthsP 
+ " months to save up.",saveMonthP,costTotal);

Formatting issue aside (that's a non-conventional way of alignment...?), since you have opted (pun unintended) to ask the user using a GUI dialog box, you should ideally present the answer back as a dialog box too. Otherwise, users may be hunting where the answers are.

Oh, and about that printf(...) usage... since you are using it, you should use a placeholder for your monthsP variable as well.

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

Don't use floating point numbers to represent monetary amounts. Prefer BigDecimal instead.

Casting a double to an int truncates the number, that is it drops all the digits after the point. So 9.999 becomes 9. Whereas you should round up.

    double months = costTotal / saveMonthP;

    //Converts the months value to an integer
    int monthsP = (int) months;

Suppose monthsP turned out 9.001. This means at the end of 9 months you'll still come up short (even if by a little), and you'll have to save for one more month.

You may find it useful to also report the amount that would be needed to be saved in the last month. So that the user might consider his options accordingly. Is that amount something he can come up with in the previous month instead? That he can borrow from friends and family? Or should he consider increasing the monthly saving amount a little to get the car a month earlier? May be he could decrease the saving amount a little to spread the saving amounts evenly among all the months? So on and so forth...

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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