14
\$\begingroup\$

This is my basic currency converter for my intro to Java class. I'm supposed to be able to convert between Yen, Dollars, Pounds and Euros using static rates. It works, but I was curious to know if I did it in the most efficient way possible. It seems quite long and looks like a huge mess. Just wanting some feedback.

import java.util.Scanner;

public class currency
{
    public currency()
    {
        char us_dollar_sym = 36;
        char pound_sym = 163;
        char yen_sym = 165;
        char euro_sym = 8364; 

        String us_dollar = "Dollars"; 
        String pound = "Pounds";
        String yen = "Yen";
        String euro = "Euros";
        double rate = 0;


        // Interface
        System.out.println("Welcome to the Currency Converter Program \n");
        System.out.println("Use the following codes to input your currency choices: \n 1 - US dollars \n 2 - Euros \n 3 - British Pounds \n 4 - Japanese Yen \n");

        // 
        System.out.println("Please choose the input currency:");
        Scanner in = new Scanner(System.in);
        int choice = in.nextInt(); 

       String inType = null;
        switch(choice) {
        case 1: inType = "US Dollars >> " + us_dollar_sym;  break;
        case 2: inType = "Euros >> " + euro_sym; break;
        case 3: inType = "British Pounds >> " + pound_sym; break;
        case 4: inType = "Japanese Yen >> " + yen_sym; break;
        default:
        System.out.println("Please restart the program & enter a number from the list.");
        return;
      }

        System.out.println("Please choose the output currency");
        int output = in.nextInt();

        System.out.printf("Now enter the input in " + inType);
        double input = in.nextDouble(); 

      if (choice == output) 
        System.out.println("Same currency no need to convert");

      if (choice == 1 && output == 2)
        { 
           double dollar_euro_rate = 0.78391;
           rate = input * dollar_euro_rate;
           System.out.printf( "%s" + input + " at a conversion rate of " + dollar_euro_rate + " Dollars to %s = %.2f\n", (char)us_dollar_sym, euro, rate);
        }
        else if (choice == 1 && output == 3){ 
           double dollar_pound_rate = 0.621484;
           rate = input * dollar_pound_rate;
           System.out.printf( "%s" + input + " at a conversion rate of " + dollar_pound_rate + " Dollars to %s = %.2f\n", (char)us_dollar_sym, pound, rate);
        }
        else if (choice == 1 && output == 4){ 
          double dollar_yen_rate = 107.174;
          rate = input * dollar_yen_rate;
          System.out.printf( "%s" + input + " at a conversion rate of " + dollar_yen_rate + " Dollars to %s = %.2f\n", (char)us_dollar_sym, yen, rate);
        }
      if (choice == 2 && output == 1)
      {
          double euro_dollar_rate = 1.27579;
          rate = input * euro_dollar_rate;
          System.out.printf( "%s" + input + " at a conversion rate of " + euro_dollar_rate + " Euros to %s = %.2f\n", (char)euro_sym, us_dollar, rate);
        }
        else if (choice == 2 && output == 3)
        {
          double euro_pound_rate = 0.792648;
          rate = input * euro_pound_rate;
          System.out.printf( "%s" + input + " at a conversion rate of " + euro_pound_rate + " Euros to %s = %.2f\n", (char)euro_sym, pound, rate);
        }
        else if (choice == 2 && output == 4)
        {
          double euro_yen_rate = 136.708;
          rate = input * euro_yen_rate;
          System.out.printf( "%s" + input + " at a conversion rate of " + euro_yen_rate + " Euros to %s = %.2f\n", (char)euro_sym, yen, rate);
        }
      if (choice == 3 && output == 1)
      {
          double pound_dollar_rate = 1.60972;
          System.out.printf( "%s" + input + " at a conversion rate of " + pound_dollar_rate + " Pounds to %s = %.2f\n", (char)pound_sym, us_dollar, rate);
        }
        else if (choice == 3 && output == 2)
        {
          double pound_euro_rate = 1.26161;
          System.out.printf( "%s" + input + " at a conversion rate of " + pound_euro_rate + " Pounds to %s = %.2f\n", (char)pound_sym, euro, rate);
        }
        else if (choice == 3 && output == 4)
        {
          double pound_yen_rate = 172.511;
          System.out.printf( "%s" + input + " at a conversion rate of " + pound_yen_rate + " Pounds to %s = %.2f\n", (char)pound_sym, yen, rate);
        }
      if (choice == 4 && output == 1)
      { 
          double yen_dollar_rate = 0.00932574;
          System.out.printf( "%s" + input + " at a conversion rate of " + yen_dollar_rate + " Yen to %s = %.2f\n", (char)yen_sym, us_dollar, rate);
        }
        else if (choice == 4 && output == 2)
        { 
          double yen_euro_rate = 0.00730615;
          System.out.printf( "%s" + input + " at a conversion rate of " + yen_euro_rate + " Yen to %s = %.2f\n", (char)yen_sym, euro, rate);
        }
        else if (choice == 4 && output == 3)
        {
          double yen_pound_rate = 0.00579135;
          System.out.printf( "%s" + input + " at a conversion rate of " + yen_pound_rate + " Yen to %s = %.2f\n", (char)yen_sym, pound, rate);
        }
       System.out.println("Thank you for using the currency converter");
    }
}
\$\endgroup\$
20
\$\begingroup\$

Your assessment of your own code is pretty close to accurate: it seems quite long and looks like a huge mess.

On the other hand, you have worked through what appear to be all the use cases, and you have comprehensively solved the problem. You have put a lot of work in to manually building out a logic tree, and calculation system, to get the conversions right. Frankly, you have done so much of it by hand, that there's little left for the system to compute. Your determination is commendable.

Addressing the mess first. A simple re-formatting of the code would do wonders for it. I took the liberty of using Eclipse, and running it through the auto-formatter:

import java.util.Scanner;

public class currency {

    public currency() {
        char us_dollar_sym = 36;
        char pound_sym = 163;
        char yen_sym = 165;
        char euro_sym = 8364;

        String us_dollar = "Dollars";
        String pound = "Pounds";
        String yen = "Yen";
        String euro = "Euros";
        double rate = 0;

        // Interface
        System.out.println("Welcome to the Currency Converter Program \n");
        System.out
                .println("Use the following codes to input your currency choices: \n 1 - US dollars \n 2 - Euros \n 3 - British Pounds \n 4 - Japanese Yen \n");

        //
        System.out.println("Please choose the input currency:");
        Scanner in = new Scanner(System.in);
        int choice = in.nextInt();

        String inType = null;
        switch (choice) {
        case 1:
            inType = "US Dollars >> " + us_dollar_sym;
            break;
        case 2:
            inType = "Euros >> " + euro_sym;
            break;
        case 3:
            inType = "British Pounds >> " + pound_sym;
            break;
        case 4:
            inType = "Japanese Yen >> " + yen_sym;
            break;
        default:
            System.out
                    .println("Please restart the program & enter a number from the list.");
            return;
        }

        System.out.println("Please choose the output currency");
        int output = in.nextInt();

        System.out.printf("Now enter the input in " + inType);
        double input = in.nextDouble();

        if (choice == output)
            System.out.println("Same currency no need to convert");

        if (choice == 1 && output == 2) {
            double dollar_euro_rate = 0.78391;
            rate = input * dollar_euro_rate;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + dollar_euro_rate + " Dollars to %s = %.2f\n",
                    (char) us_dollar_sym, euro, rate);
        } else if (choice == 1 && output == 3) {
            double dollar_pound_rate = 0.621484;
            rate = input * dollar_pound_rate;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + dollar_pound_rate + " Dollars to %s = %.2f\n",
                    (char) us_dollar_sym, pound, rate);
        } else if (choice == 1 && output == 4) {
            double dollar_yen_rate = 107.174;
            rate = input * dollar_yen_rate;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + dollar_yen_rate + " Dollars to %s = %.2f\n",
                    (char) us_dollar_sym, yen, rate);
        }
        if (choice == 2 && output == 1) {
            double euro_dollar_rate = 1.27579;
            rate = input * euro_dollar_rate;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + euro_dollar_rate + " Euros to %s = %.2f\n",
                    (char) euro_sym, us_dollar, rate);
        } else if (choice == 2 && output == 3) {
            double euro_pound_rate = 0.792648;
            rate = input * euro_pound_rate;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + euro_pound_rate + " Euros to %s = %.2f\n",
                    (char) euro_sym, pound, rate);
        } else if (choice == 2 && output == 4) {
            double euro_yen_rate = 136.708;
            rate = input * euro_yen_rate;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + euro_yen_rate + " Euros to %s = %.2f\n", (char) euro_sym,
                    yen, rate);
        }
        if (choice == 3 && output == 1) {
            double pound_dollar_rate = 1.60972;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + pound_dollar_rate + " Pounds to %s = %.2f\n",
                    (char) pound_sym, us_dollar, rate);
        } else if (choice == 3 && output == 2) {
            double pound_euro_rate = 1.26161;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + pound_euro_rate + " Pounds to %s = %.2f\n",
                    (char) pound_sym, euro, rate);
        } else if (choice == 3 && output == 4) {
            double pound_yen_rate = 172.511;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + pound_yen_rate + " Pounds to %s = %.2f\n",
                    (char) pound_sym, yen, rate);
        }
        if (choice == 4 && output == 1) {
            double yen_dollar_rate = 0.00932574;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + yen_dollar_rate + " Yen to %s = %.2f\n", (char) yen_sym,
                    us_dollar, rate);
        } else if (choice == 4 && output == 2) {
            double yen_euro_rate = 0.00730615;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + yen_euro_rate + " Yen to %s = %.2f\n", (char) yen_sym,
                    euro, rate);
        } else if (choice == 4 && output == 3) {
            double yen_pound_rate = 0.00579135;
            System.out.printf("%s" + input + " at a conversion rate of "
                    + yen_pound_rate + " Yen to %s = %.2f\n", (char) yen_sym,
                    pound, rate);
        }
        System.out.println("Thank you for using the currency converter");
    }
}

OK, let's go through that:

  • Java classes should have a capital letter for the name. Call it 'Currency'.
  • Java is object oriented, which means you should actually have objects. You have none, and you embed all the logic in the constructor of the class... not good.
  • You repeat yourself a lot
  • You repeat yourself a lot
  • You repeat yourself a lot

Let's work on a strategy, without rewriting the whole thing for you. Let's start with a currency convertion tool that is object oriented. The first object, is a currency, called Currency.

public class Currency {
}

Now, with any conversion tool, the trick is to have as little redundancy as possible. The way to do it in this case is to have a common currency, and the exchange rates from all the other currencies to the common one. Since you list USD as the first currency, we will go with that, and the following table:

        USD
USD     1.0
Euro    0.78391
GBP     0.621484
Yen   107.174

Now, we flesh out each currency to have the rate to the common currency:

public class Currency {
    private final String name;
    private final double rate;
    public Currency(String name, double rate) {
        this.name = name;
        this.rate = rate;
    }
}

Then, create instances (in an array?) like:

Currency[] currencies = {
    new Currency("USD", 1.0),
    new Currency("Euro", 0.78391),
      ....
};

Now, if we want to convert from Euro to Yen, we convert first from Euro to USD, then USD to Yen. You can do it by dividing the exchange rates:

Do some user input:

double sourceAmount = ....; // some source amount
String sourceCurrency = ...; // some currency 
String targetCurrency = ....; // you get the idea.

Search the Currency instances for the right rates:

double rateToDollars = ....; // find the rate from source to dollars (may be 1.0).
double rateToTarget = ....; // find the rate from target to dollars (may be 1.0)

calculate the converted amount:

double targetAmount = (sourceAmount / rateToDollars) * rateToTarget;

That about sums up what the code should look like.

\$\endgroup\$
  • 1
    \$\begingroup\$ I work in international payments processing (hence my clicking on this question), and the base currency concept is something we use in industry. On topic, expanding the Currency class to also contain the (char) code and currency name wouldn't be a bad idea, either ;) \$\endgroup\$ – Noah Oct 20 '14 at 4:11
  • 2
    \$\begingroup\$ Hey @Noah, I wish I could say the idea was original.... but I was introduced to the concept in the early 90's when I first entered the industry (I have mostly worked in development in the financial sector), and I actualy wrote very similar concepts myself when preparing an international bank for European monetary union in 1998 with go-live in 1999. Static currency values and all. Admittedly it was more robust... but.... \$\endgroup\$ – rolfl Oct 20 '14 at 10:00
  • 2
    \$\begingroup\$ Coming from a science background into finance I like to introduce also a Money class with Money.Currency and Money.Amount (dimensional analysis). This allows you to create a Money.Multiple(FxRate), with a currency check (since 50% of the time I get my crosses the wrong way round). THEN UNIT TEST THE BEJESUS out of the two classes. Of course as a C# dev, I use * operators. \$\endgroup\$ – Aron Oct 20 '14 at 16:59
8
\$\begingroup\$

I remember when I tried building a program in a constructor for a class, but it's generally best to keep constructors as short as possible. They should allocate memory and initialize variables. They should not ask the user for input. Constructors should be independent of anything outside the constructor.

If you have a class called currency, then it should manage objects that represent a currency. What this class seems to be is a currency_converter.

In the following code:

  System.out.println("Please choose the output currency");
  int output = in.nextInt();

  System.out.printf("Now enter the input in " + inType);
  double input = in.nextDouble(); 

  if (choice == output) 
      System.out.println("Same currency no need to convert");

Why do you wait until after the user enters the input amount to say that there's no need to convert? Why not tell them that immediately? Also, why do you continue after telling them that you won't do anything? You could just return instead. Or put the input in a loop that demands a valid response.

When you want to give a number special meaning, you should use an enum. All the == 1, etc. in your code should be handled with Enum values instead.

In terms of your if/else if construct, you can create a two-dimensional array where each cell is a conversion rate from a currency to a currency. The row and column tell you the source and target currencies. The advantage of the two-dimensional array is that it only has to do the conversion once, so there is less chance of a rounding error. For example, I just tried doing a conversion on oanda.com from USD to EUR to GBP, but I got a different result from when I converted directly from USD to GBP. The disadvantage is that it is harder to set up. You'd use it something like this:

target_amount = source_amount * conversion_factor_of[source_currency][target_currency];

In general, you should break up your existing code into multiple functions. For example, maybe you put each input gathering into its own function. Or come up with a way to standardize them and put all into the same function.

\$\endgroup\$

protected by Jamal May 14 '16 at 20:23

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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