1
\$\begingroup\$

The code already works, but I just want tips for improving.

import javax.swing.JOptionPane;
import java.text.DecimalFormat;
public class Calculator
{
    public static void main(String[] args)
    {
        double initialPrice = 0.00;
        double commissionRate = 0;
        double discountRate = 0;
        initialPrice = Double.parseDouble(JOptionPane.showInputDialog(null, "What is the initial price total of the sale?"));
        commissionRate = Double.parseDouble(JOptionPane.showInputDialog(null, "What is the percentage amount of the sales commission? \nFor example for 20%, type 20."));
        discountRate = Double.parseDouble(JOptionPane.showInputDialog(null, "What is the customer's discount rate? \nFor example for a 15% discount, type 15."));
        JOptionPane.showMessageDialog(null, "The final cost to the customer is \n$" + computeFinalPrice(initialPrice, commissionRate, discountRate) + ".");
    }
    public static String computeFinalPrice (double initialPrice, double commissionRate, double discountRate)
    {
        double finalPrice;
        DecimalFormat d = new DecimalFormat("0.00");
        finalPrice = (initialPrice*(commissionRate+100)*.01)*((100-discountRate)*.01);      
        return (d.format(finalPrice));
    }
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Interesting, consider the following:

  • For extra DRYness, you could have created a helper function to get a double

    private double getDouble( String msg )
    
  • computeFinalPrice should probably be private

  • If you wanted computeFinalPrice to be re-usable, then it probably makes more sense to return a double and keep it public
  • A company would probably rather compute commission after discount, otherwise sales people will get really excited about giving 99% discounts ;)
  • Either you initialize doubles with zeroes like in main() or you don't like in computeFinalPrice.
  • As mentioned by another reviewer, consider Long instead of Double, as Doubles can have rounding issues.
  • I would write

    finalPrice = (initialPrice*(commissionRate+100)*.01)*((100-discountRate)*.01);  
    

    as

    `finalPrice = initialPrice*(commissionRate+100)*.01*(100-discountRate)*.01;`  
    

    if I felt like messing with the code reviewer I might even

    `finalPrice = initialPrice*(commissionRate+100)*(100-discountRate)*.0001;`  
    
\$\endgroup\$
1
  • \$\begingroup\$ Good point about making it private. It's for class, so I'm locked into applying the discount after the commission. I agree it makes little sense from a business perspective. \$\endgroup\$
    – Brian
    Commented Mar 11, 2013 at 23:32
2
\$\begingroup\$

Just a quick note: Don't use floating point variables where you may need exact results:

Fortunately Java has BigDecimal.

\$\endgroup\$
1
  • \$\begingroup\$ I'm only a week into my first class so I'm still learning the ropes. I looked into BigDecimal after a few google searches. Since the program only takes a singular input for the output and only compounds once, I figured that the accuracy wouldn't matter too much. However, point taken. I'll school myself up on BigDecimal and use it from now on. Thanks for the feedback! \$\endgroup\$
    – Brian
    Commented Mar 12, 2013 at 2:09

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.