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\$
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
    Mar 11 '13 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
    Mar 12 '13 at 2:09

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.