5
\$\begingroup\$

I've made this program to do a few integer calculations. Do the methods for converting the numbers look good? Do you have any other basic tips?

/* Dollar Value of Coins, Investment Compounded Annually
 * B, Morris
 */

import java.util.*;
import java.lang.*;
import java.text.*;
public class HW3_DollarValuOfCoinsInvestmentCompoundedAnnually {
public static void main(String args[]) {
Scanner keyboard = new Scanner(System.in);
NumberFormat priceFormat = NumberFormat.getCurrencyInstance();

//Dollar value of coins

System.out.println("How many quarters do you have?");
double quarters = keyboard.nextDouble();

System.out.println("How many dimes do you have?");
double dimes = keyboard.nextDouble();

System.out.println("How many nickles do you have?");
double nickles = keyboard.nextDouble();

quarters = quarters * (0.25);
dimes = dimes * (0.10);
nickles = nickles * (0.05);
double total =(quarters + dimes + nickles);
System.out.println("You have: " + priceFormat.format(total));


//Investment Compounded Annually

System.out.println("What is the initial investment?");
double investment = keyboard.nextDouble();

System.out.println("At what intrest rate is the intrest compounded annually?");
double intrestRate = keyboard.nextDouble();

double futureValueFive = investment * (Math.pow((1 + intrestRate), (double) 5));
System.out.println("In five years the investment will be worth : " + priceFormat.format(futureValueFive));

double futureValueTen = investment * (Math.pow((1 + intrestRate), (double) 10));
System.out.println("In ten years the investment will be worth : " + priceFormat.format(futureValueTen));

double futureValueTwenty = investment * (Math.pow((1 + intrestRate), (double) 20));
System.out.println("In twenty years the investment will be worth : " + priceFormat.format(futureValueTwenty));


}
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ In general, it is better to store financial values in [decimal]( msdn.microsoft.com/en-us/library/364x0z75.aspx). In this case it does not really matter because you're doing other floating point operations, but it is worth keeping it in mind \$\endgroup\$ Commented Oct 4, 2014 at 6:10

3 Answers 3

8
\$\begingroup\$

Choose the right types

You used double for the coin types, for example:

System.out.println("How many quarters do you have?");
double quarters = keyboard.nextDouble();
quarters = quarters * (0.25);

To the question "how many quarters ...", it's logical to get an integer (whole number) as the answer, not a double.

I can guess that you choose the double type because you want to use the quarters variable for two different purposes:

  • Count the quarters
  • Count the dollar value of the quarters

These are conflicting meanings, and the right thing to do is to not mix them, for example:

int quarters = keyboard.nextInt();
double dollarValueOfQuarters = quarters * .25;

The Single Responsibility Principle

The main is doing too much: it has too many responsibilities:

  1. Calculate the dollar value of coins
  2. Calculate the compound interest

It would be better to split the method into, and give them a name according to their main responsibility, for example:

private static void calculateDollarValueOfCoins(Scanner scanner, NumberFormat moneyFormat) { ... }

private static void calculateCompoundInterest(Scanner scanner, NumberFormat moneyFormat) { ... }

public static void main(String args[]) {
    Scanner scanner = new Scanner(System.in);
    NumberFormat moneyFormat = NumberFormat.getCurrencyInstance();

    calculateDollarValueOfCoins(scanner, moneyFormat);
    calculateCompoundInterest(scanner, moneyFormat);
}

Now the responsibilities are clearly separated. I also renamed some variables to better match their purposes:

  • scanner instead of keyboard, because you don't really "scan" things from a keyboard. A scanner is a more abstract concept than a keyboard: for all you care, the input values could come as radio signals from the moon, as long as it implements the Scanner's API, your method can work.
  • moneyFormat instead of priceFormat, which works for both responsibilities nicely: the dollar value of your coins is certainly not a "price", and the worth of your investment is not exactly a "price". They are both about money, and formatting money, so this more general name seems appropriate.

Modeling coins

It might be a good idea to model coins using an enum:

enum Coin {
    NICKLE(.05),
    DIME(.1),
    QUARTER(.25);

    private final double value;

    Coin(double value) {
        this.value = value;
    }
}

And to add a helper class for adding coins:

private static class CoinAdder {
    private double value = 0;

    CoinAdder addCoins(Coin coin, int number) {
        value += coin.value * number;
        return this;
    }

    public double getValue() {
        return value;
    }
}

This way, the calculateDollarValueOfCoins method I suggested above can be implemented in a somewhat more natural way, and without embedding the dollar value of coins in it:

private static void calculateDollarValueOfCoins(Scanner scanner, NumberFormat moneyFormat) {
    System.out.println("How many quarters do you have?");
    int quarters = scanner.nextInt();

    System.out.println("How many dimes do you have?");
    int dimes = scanner.nextInt();

    System.out.println("How many nickles do you have?");
    int nickles = scanner.nextInt();

    double total = new CoinAdder()
            .addCoins(Coin.QUARTER, quarters)
            .addCoins(Coin.DIME, dimes)
            .addCoins(Coin.NICKLE, nickles)
            .getValue();
    System.out.println("You have: " + moneyFormat.format(total));
}

Now we have separated the responsibilities even further: calculateDollarValueOfCoins doesn't know anymore the value of the different types of coins, and it doesn't know how to add them. Those responsibilities are delegated to the Coin enum and the CoinAdder class, which is a good thing.

Quick tips

Instead of this:

quarters = quarters * (0.25);

Better:

quarters *= 0.25;

You can write 0.25 as .25.

You can write 5. instead of (double) 5.

Suggested implementation

Putting the above suggestions together, something like this would be better:

class InterestCalculator {

    enum Coin {
        NICKLE(.05),
        DIME(.1),
        QUARTER(.25);

        private final double value;

        Coin(double value) {
            this.value = value;
        }
    }

    private static class CoinAdder {
        private double value = 0;

        CoinAdder addCoins(Coin coin, int number) {
            value += coin.value * number;
            return this;
        }

        public double getValue() {
            return value;
        }
    }

    private static void calculateDollarValueOfCoins(Scanner scanner, NumberFormat moneyFormat) {
        System.out.println("How many quarters do you have?");
        int quarters = scanner.nextInt();

        System.out.println("How many dimes do you have?");
        int dimes = scanner.nextInt();

        System.out.println("How many nickles do you have?");
        int nickles = scanner.nextInt();

        double total = new CoinAdder()
                .addCoins(Coin.QUARTER, quarters)
                .addCoins(Coin.DIME, dimes)
                .addCoins(Coin.NICKLE, nickles)
                .getValue();
        System.out.println("You have: " + moneyFormat.format(total));
    }

    private static void calculateCompoundInterest(Scanner scanner, NumberFormat moneyFormat) {
        System.out.println("What is the initial investment?");
        double investment = scanner.nextDouble();

        System.out.println("At what intrest rate is the intrest compounded annually?");
        double intrestRate = scanner.nextDouble();

        double futureValueFive = investment * Math.pow(1 + intrestRate, 5.);
        System.out.println("In five years the investment will be worth : " + moneyFormat.format(futureValueFive));

        double futureValueTen = investment * Math.pow(1 + intrestRate, 10.);
        System.out.println("In ten years the investment will be worth : " + moneyFormat.format(futureValueTen));

        double futureValueTwenty = investment * Math.pow(1 + intrestRate, 20.);
        System.out.println("In twenty years the investment will be worth : " + moneyFormat.format(futureValueTwenty));
    }

    public static void main(String args[]) {
        Scanner scanner = new Scanner(System.in);
        NumberFormat moneyFormat = NumberFormat.getCurrencyInstance();

        calculateDollarValueOfCoins(scanner, moneyFormat);
        calculateCompoundInterest(scanner, moneyFormat);
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Hi why dint you use an interface? Any reasons particularly? Why are you declaring value both in Enum and coinaddr \$\endgroup\$
    – fscore
    Commented Oct 4, 2014 at 13:40
  • 1
    \$\begingroup\$ It's simple, I guess. I don't claim it's perfect. Also keep in mind that it's a beginner question, I did't want to overengineer it too much. As per CoinAdder.value, notice that it means something completely different from Coin.value \$\endgroup\$
    – janos
    Commented Oct 4, 2014 at 17:37
6
\$\begingroup\$

Instead of calculating the total value of the coins in main():

quarters = quarters * (0.25);
dimes = dimes * (0.10);
nickles = nickles * (0.05);

consider using separate methods for each type of coin:

public static double getQuarters(int quarters) {
    return quarters * 0.25;
}

public static double getDimes(int dimes) {
    return dimes * 0.10;
}

public static double getNickles(int nickles) {
    return nickles * 0.05;
}

Try not to keep everything in main(), especially if your program starts to get larger. Try to decompose to multiple smaller methods that each have a single responsibility.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I would rephrase that: you should avoid putting everything in main(). Or even add: try to decompose to multiple smaller methods that each have a single responsibility ;-) \$\endgroup\$
    – janos
    Commented Oct 20, 2014 at 5:58
2
\$\begingroup\$

I would also make an interface of these methods:

interface investmentCompound
{
    double getQuarters(double numOfQuarters);   
    double getDimes(double numOfDimes);
    double getNickles(double numOfNickles);
    double performCalculations();
    double formatPrice(double priceToFormat);
    ...

}
  • The above will give your class a much better design and help you better code as well. Always program in such a way that when anyone reads your code, the method and variable names are self-explanatory on what your class is doing.

  • Give your main() function the minimal capability to just call methods and variables instead of declaring everything there.

  • In the real world, you will always have an API i.e. getters and setters and methods like how I mentioned in the interface above. This is better practice, so when someone wants to call your code, they should be able to use the methods.

  • Since you are going to use numOfNickles, numOfQuarters, numOfDimes variable names several times, it's best to declare that at class level, so declare them as class variables instead of in main().

\$\endgroup\$
2
  • \$\begingroup\$ The arguments for the getters should be an integer type. \$\endgroup\$
    – Jamal
    Commented Oct 4, 2014 at 3:39
  • 2
    \$\begingroup\$ In what way does that particular collection of methods constitute a coherent interface? Would you ever have more than one implementation of such an interface? If not, why bother declaring it as an interface? \$\endgroup\$ Commented Oct 4, 2014 at 6:53

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