My Basic Currency Converter program gives bad output

Question

So, The code SHOULD convert currencies, but it doesn't do it correctly. I have a variable k(Croatian kuna... 1 EURO = 7.5 KUNA)which is 1 and for example, if I want to convert 1 euro to 1 dollar, the program multiplies the amount (1) by 7.5, then I have that amount of euros in KUNA, and that Works. But, when I go to divide that result (7.5) with 6.3(1 DOLLAR IS 6.3 KUNA), I get the same number.

import java.util.Scanner;

public class Conv {
    private double rez;
    private double rez2;
    private double svota;
    Scanner ul = new Scanner(System.in);

    public void PretvorbaInKunu(double y) {
        System.out.print("Insert amomunt: ");
        svota = ul.nextDouble();
        rez2 = svota*y;

    }

    public void PR2(double x) {
        rez = getRez2() / x;
    }

    public double getRez() {
        return rez;
    }
    public double getRez2() {
        return rez2;
    }
    public double getSvota() {
        return svota;
    }

}
import java.util.Scanner;

//Currency Converter

public class Vjezbica {
    public static void main(String[] args) {
        double e = 7.5;
        double d = 6.3;
        double p = 9.5;
        double k = 1.0;
        Conv more = new Conv();
        Scanner in = new Scanner(System.in);

        System.out.print("\t\tCurrency converter\nIz (e,p,d,k) - ");
        String iz = in.next();
        switch(iz) {
        case "e":
            more.PretvorbaInKunu(e);
            break;
        case "d":
            more.PretvorbaInKunu(d);
            break;
        case "p":
            more.PretvorbaInKunu(p);
            break;
        case "k":
            more.PretvorbaInKunu(k);
            break;
        }
        System.out.println(more.getRez2());

        System.out.print(" To  ");
        String u = in.next();
        switch(u) {
        case "e":
            more.PR2(e);
        case "d":
            more.PR2(d);
        case "p":
            more.PR2(p);
        case "k":
            more.PR2(k);
        }
        System.out.println(more.getSvota() + " " + iz + " is " + more.getRez() + " " + u);
    }
}

Answer 1

First of all - mistake was, as already pointed out by Tobias, the missing "break;" But there are other mistakes in the code, e.g. Scanner is not closed.

I would also suggest to improve code quality. You could write an enum type that performs just calculation (no input - input should be separated). You can easily define currencies with different enum constructors then.

There was also discussion above if double or BigDecimal should be used. BigDecimal is an "expensive" type. I am wondering if float isn't enough, because currency values usually contain a precision of 2 digits - and if you are not dealing with very high amounts, a 32bit float type should already suffer.

I'd rewrite it as follows:

import java.util.Scanner;

//Currency Converter

public class Vjezbica {

    private static enum Currency {
        EURO(7.5), KUNA(1), DOLLAR(6.3), P(9.3);

        private double cr;

        Currency(double conversionRate) {
            this.cr = conversionRate;
        }

        // precision: 2digits
        public float fromKuna(double kuna) {
            return (int) (((kuna) / cr) * 100) / 100f;
        }

        public float toKuna(double foreignCurrency) {
            return (int) (((foreignCurrency) * cr) * 100) / 100f;
        }
    }

    private static Currency readCurrency() {
        Scanner in = new Scanner(System.in);
        System.out.print("\t\tCurrency converter\nIz (e,p,d,k) - ");
        String iz = in.next();
        in.close();

        switch (iz) { // no break required when returning directly
        case "e":
            return Currency.EURO;
        case "d":
            return Currency.DOLLAR;
        case "p":
            return Currency.P;
        case "k":
            return Currency.KUNA;
        default:
            throw new IllegalArgumentException("invalid input: " + iz);
        }
    }

    private static float readAmount() {
        Scanner ul = new Scanner(System.in);
        float svota = ul.nextFloat();
        ul.close();
        return svota;
    }

    public static void main(String[] args) {

        System.out.print(" From  ");
        Currency c1 = readCurrency();

        System.out.print("Insert amomunt: ");
        float svota = readAmount();

        float amountInKuna = c1.toKuna(svota);

        System.out.print(" To  ");
        Currency c2 = readCurrency();

        float amountInC2 = c2.fromKuna(amountInKuna);
        System.out.println(svota + " " + c1.toString() + " is " + amountInC2 + " " + c2.toString());

    }


}

Not considered: conversion does first convert from X to Kuna, and then from Kuna to Y. In both conversions, we are performing an inprecise rounding (flooring instead of correct rounding, and flooring twice instead of just once).

Based on this suggestion, it would be even better if you calculate a precise conversion rate which allows direct conversion from X to Y in the end, and you perform a round operation on the final result then.

Answer 2

The problem is in your second switch-case statement: You need to add a break at the end of every case.

If you change the class Vjezbica like this it should work:

import java.util.Scanner;

//Currency Converter

public class Vjezbica {

    public static void main(String[] args) {
        double e = 7.5;
        double d = 6.3;
        double p = 9.5;
        double k = 1.0;
        Conv more = new Conv();
        Scanner in = new Scanner(System.in);

        System.out.print("\t\tCurrency converter\nIz (e,p,d,k) - ");
        String iz = in.next();
        switch (iz) {
            case "e":
                more.PretvorbaInKunu(e);
                break;
            case "d":
                more.PretvorbaInKunu(d);
                break;
            case "p":
                more.PretvorbaInKunu(p);
                break;
            case "k":
                more.PretvorbaInKunu(k);
                break;
        }
        System.out.println(more.getRez2());

        System.out.print(" To  ");
        String u = in.next();
        switch (u) {
            case "e":
                more.PR2(e);
                break;//added break here
            case "d":
                more.PR2(d);
                break;//added break here
            case "p":
                more.PR2(p);
                break;//added break here
            case "k":
                more.PR2(k);
                break;//added break here
        }
        System.out.println(more.getSvota() + " " + iz + " is " + more.getRez() + " " + u);

        //you should also close the scanner at the end...
        in.close();
    }
}