Qfyilyi

This program calculates with numbers from different number systems and outputs the result in the desired number system.

You call it like that:

java Calculator <operator> <number> <base> <otherNumber> <base> (solutionBase)

Unfortunately it does not work with floating point numbers.

Example:

$java Calculator + 5234 7 FABCD43 16 3

200022201200110011

My gut feeling says me that my code is quite ugly but it doesnt tell me how to improve it. Do you have some hints for me how to make this more beautiful?

public class Calculator {

    public static void main(String args) {

        // display usage if user wants so

        if (args[0].contains("help")) {

            displayHelp();

            return;

        }



        // parse arguments

        String operator = args[0];

        String number1 = args[1];

        int baseOfNumber1 = Integer.parseInt(args[2]);

        String number2 = args[3];

        int baseOfNumber2 = Integer.parseInt(args[4]);

        int baseOfSolution = 0;

        if (args.length == 6) {

            baseOfSolution = Integer.parseInt(args[5]);

        }



        int number1Dec = randomBaseToDecimal(number1, baseOfNumber1);

        int number2Dec = randomBaseToDecimal(number2, baseOfNumber2);



        // calculate and print out

        int solutionDec = 0;

        if (operator.equals("+")) {

            solutionDec = number1Dec + number2Dec;

        } else if (operator.equals("-")) {

            solutionDec = number1Dec - number2Dec;

        } else if (operator.equals("x")) {

            solutionDec = number1Dec * number2Dec;

        } else if (operator.equals("/")) {

            solutionDec = number1Dec / number2Dec;

        }



        if (args.length == 6) {

            System.out.println(decimalToRandomBase(solutionDec,

              baseOfSolution));

        } else {

            System.out.println(solutionDec);

        }

    }



    private static int randomBaseToDecimal(String number, int base) {

        int result = 0;

        for (int i = 0; i < number.length(); i++) {

            int digit = Character.getNumericValue(number.charAt(i));

            result = base * result + digit;

        }

        return result;

    }



    // works only till base 16

    private static String decimalToRandomBase(int number, int base) {

        StringBuilder finalNumber = new StringBuilder();

        while (number != 0) {

            if ((number % base) > 9) {

                switch ((number % base)) {

                    case 10: finalNumber.append("A"); break;

                    case 11: finalNumber.append("B"); break;

                    case 12: finalNumber.append("C"); break;

                    case 13: finalNumber.append("D"); break;

                    case 14: finalNumber.append("E"); break;

                    case 15: finalNumber.append("F"); break;

                }

            } else {

                finalNumber.append(number % base);

            }

            number = number / base;

        }

        return new StringBuilder(finalNumber).reverse().toString();

    }



    private static void displayHelp() {

        System.out.println("This program calculates with numbers of different bases");

        System.out.println("Example: ");

        System.out.println("java Calculator + 34 5 554 6");

        System.out.println("You can also specify the base of the output number as the last argument:");

        System.out.println("java Calculator + 34 5 554 6 2");

    }

}

Abstraction

One thing that makes code more elegant is abstraction. Consider adding something like

class ArbitraryBaseNumber {



    private final int number;

    private final int base;



    public ArbitraryBaseNumber(int number, int base) {

        this.number = number;

        this.base = base;

    }



    public static ArbitraryBaseNumber valueOf(String number, String base) {

         int radix = Integer.parseInt(base);

         int n = Integer.parseInt(number, radix);

         return new ArbitraryBaseNumber(n, radix);

    }



    public String toString(int radix) {

         return Integer.toString(number, radix);

    }



    @Override

    public String toString() {

        return toString(base);

    }



    public int toInteger() {

         return number;

    }



    public getBase() {

        return base;

    }



}

I think that arbitrary is more descriptive than random. Usually when something is random in computer science, it is created by a random number generator. But you're not doing that here. Random may be correct English, but it makes for confusing code here. Arbitrary does not cause that same confusion.

Now, code like

        String number1 = args[1];

        int baseOfNumber1 = Integer.parseInt(args[2]);

        String number2 = args[3];

        int baseOfNumber2 = Integer.parseInt(args[4]);

Can be written

        ArbitraryBaseNumber operand1 = ArbitraryBaseNumber.valueOf(args[1], args[2]);

        ArbitraryBaseNumber operand2 = ArbitraryBaseNumber.valueOf(args[3], args[4]);

And code like

        int number1Dec = randomBaseToDecimal(number1, baseOfNumber1);

        int number2Dec = randomBaseToDecimal(number2, baseOfNumber2);

could just be

        int number1Dec = operand1.toInteger();

        int number2Dec = operand2.toInteger();

Although I would actually approach this differently.

I think that it's a bit odd to call Java integers Dec. They are actually stored in binary. They are often converted to strings as decimal numbers, but they aren't stored that way.

Delegation

When you have something like

      // calculate and print out

        int solutionDec = 0;

        if (operator.equals("+")) {

            solutionDec = number1Dec + number2Dec;

        } else if (operator.equals("-")) {

            solutionDec = number1Dec - number2Dec;

        } else if (operator.equals("x")) {

            solutionDec = number1Dec * number2Dec;

        } else if (operator.equals("/")) {

            solutionDec = number1Dec / number2Dec;

        }

Consider writing a method.

    public int calculate(char operator, int a, int b) {

        switch (operator) {

            case '+':

                return a + b;

            case '-':

                return a - b;

            case '*':

            case 'x':

                return a * b;

            case '/':

                return a / b;

            default:

                throw new IllegalArgumentException("Unrecognized operator:  [" + operator + "]");

        }

    }

As previously suggested, we can use a switch with a default behavior of throwing an exception. This can save a lot of operator.equals calls.

I added '*' accidentally but then kept it as more intuitive. This way, it will accept either * or x.

By using return, we can exit from both the switch and the method. This saves us also having to write break; each time.

Adding to the exception message makes it easier to tell where the operator begins and ends. Sometimes that gets lost. For example, if someone enters a period where the operator should be.

I changed from a String operator to a character operator. You would use it like

    public int calculate(String operator, ArbitraryBaseNumber operand1, ArbitraryBaseNumber operand2) {

        return calculate(operator.charAt(0), operand1.toInteger, operand2.toInteger);

    }

which you would call like

        int solution = calculate(operator, operand1, operand2);

In the background, I would expect this to make the evaluation more efficient, since all your operators are single characters.

Putting it together

        ArbitraryBaseNumber operand1 = ArbitraryBaseNumber.valueOf(args[1], args[2]);

        ArbitraryBaseNumber operand2 = ArbitraryBaseNumber.valueOf(args[3], args[4]);



        int solution = calculate(args[0], operand1, operand2);



        String result;

        if (args.length == 6) {

            result = Integer.toString(solution, Integer.parseInt(args[5]));

        } else {

            result = Integer.toString(solution);

        }



        System.out.println(result);

That's the entire body of the main method except for the part that displays your help message.

I moved the parsing of the operator and the base of the solution later in the method. The operator isn't a big deal either way. The problem with the base of the solution is that you created parallel logic. You checked args.length == 6 in two places. This merges that into one check, which is generally more reliable. If you do have to separate the logic, consider something like

        Integer solutionBase = null;

        if (args.length == 6) {

            solutionBase = Integer.parseInt(args[5]);

        }

and then later

        String result = (solutionBase == null) ? Integer.toString(solution)

                                               : Integer.toString(solution, solutionBase);

That tends to be more robust in regards to future changes (e.g. adding another argument or allowing an arbitrary number of operators and operands).

Or in this case, you might do

        int solutionBase = 10;

        if (args.length == 6) {

            solutionBase = Integer.parseInt(args[5]);

        }

And then at the end

        System.out.println(Integer.toString(solution, solutionBase));

Now we have the same logic at the end regardless of the number of arguments.

Reinventing the wheel

It is of course possible that you wanted to write your own versions of parseInt and toString. You can certainly do that (using the reinventing-the-wheel tag would tell us that's what you're doing). But I would still suggest making them match the original versions' method signatures unless you have a strong reason to change them. Then you could just replace the standard versions with your versions in this code.

Main feedback has already been given by @AJNeufeld and this post is not about the performance of your program but rather other aspects.

You should try putting a check before you access an index in an array and slightly change your if block from this,

if (args[0].contains("help")) {

to,

if (args.length == 0 || args[0].contains("help") || args.length < 5) {

as the former will run into ArrayIndexOutOfBoundsException if no argument is passed. Also it would be helpful to call the displayHelp() method in case no argument (args.length == 0) was passed so the user knows the usage of program.

Also, for safely accessing array indexes, you should put another OR condition args.length < 5 which will ensure at least five parameters are passed, else again you may run into ArrayIndexOutOfBoundsException.

These checks should make the program a little more safer.

If your goal is to implement the conversion functions yourself:

1. You are repeating number % base 3 times. Once in the if statement, once in the switch statement, and once in finalNumber.append(). You should do the calculation once, and store it as a local variable.



2. 
   

   As noted in the comment, decimalToRandomBase() only works up to base 16. You could expand this to base 36 by:

   
   
   
   
   
   • calculating the character to append, 'A' + (number % base - 10), instead of using a switch statement, or
   
   
   • Using Character.forDigit(value, radix) which is the opposite of the Character.getNumericValue() function. For values 10 and greater, it will return lower case letters, however.
   
   
   
   



3. You already have a StringBuilder; you don't need to create a new StringBuilder(finalNumber) in order to .reverse().toString(). Simply finalNumber.reverse().toString() will work.

If your goal isn't to implement the conversion functions yourself, you can replace randomBaseToDecimal and decimalToRandomBase with:

• 
  Integer.parseInt(str, radix) - string to int with arbitrary base, and


• 
  Integer.toString(i, radix) - int to string with arbitrary base

You check twice for a 6th argument: the base to display the answer in. Once to convert it to an integer (if present), and a second time when printing the answers. If you initialize the baseOfSolution to 10:

    int baseOfSolution = 10;

    if (args.length == 6) {

        baseOfSolution = Integer.parseInt(args[5]);

    }

then you don't have to check for the existence of the 6th argument to decide between printing out the value in baseOfSolution, or base 10. You can simply print the solution in baseOfSolution.

This chain of if/elseif statements can be replaced by a switch statement.

    int solutionDec = 0;

    if (operator.equals("+")) {

        solutionDec = number1Dec + number2Dec;

    } else if (operator.equals("-")) {

        solutionDec = number1Dec - number2Dec;

    } else if (operator.equals("x")) {

        solutionDec = number1Dec * number2Dec;

    } else if (operator.equals("/")) {

        solutionDec = number1Dec / number2Dec;

    }

If the operator isn't one of the listed operations, the program simply outputs zero? That is unexpected behaviour! This would be better:

    int solutionDec = 0;

    switch (operator) {

        case "+": 

            solutionDec = number1Dec + number2Dec;

            break;

        case "-":

            solutionDec = number1Dec - number2Dec;

            break;

        case "x":

            solutionDec = number1Dec * number2Dec;

            break;

        case "/":

            solutionDec = number1Dec / number2Dec;

            break;

        default:

            throw new IllegalArgumentException("Unrecognized operator: "+operator);

    }

The test if (args[0].contains("help")) is odd. Is it really the intention to match words like "unhelpful" in addition to "help"? Or was this supposed to be if (args[0].equals("help"))? Or perhaps if (args[0].equalsIgnoreCase("help"))?

Your help is less than helpful. It doesn't describe which of the arguments are the values and which are the bases. It would also be useful to advise the user as to which operations are supported; many might try "*" instead of "x" for multiplication.