Simple calculator that seems inefficient

Question

I have a simple calculator program that needs some reviewing. Is this as efficient as it can get, or is there a better way?

public static void main(String[] args) {
    Scanner sc = new Scanner(System.in);
    System.out.println("Welcome to the Calculator!");
    do {
        System.out.println("\nType a calculation:");
        String s = sc.nextLine();
        try {
            System.out.println("\nThe answer is: " + calculate(s));
        } catch(Exception e) {
            System.out.println("Oops! Illegal calculation.");
        }
    } while(doAgain(sc));
    System.out.println("\nThank you for using the calculator!");
    sc.close();
}

private static boolean doAgain(Scanner sc) {
    while(true) {
        System.out.println("\nAgain?");
        String s = sc.nextLine();
        if(s.equalsIgnoreCase("y") || s.equalsIgnoreCase("yes") || s.equalsIgnoreCase("t") || s.equalsIgnoreCase("true"))
            return true;
        else if(s.equalsIgnoreCase("n") || s.equalsIgnoreCase("n") || s.equalsIgnoreCase("f") || s.equalsIgnoreCase("false"))
            return false;
    }
}

public static Double calculate(String s) throws IllegalArgumentException {
    IllegalArgumentException exception = new IllegalArgumentException("Illegal Calculation.");
    String a = "";
    ArrayList<Double> o = new ArrayList<Double>(s.length());
    ArrayList<String> z = new ArrayList<String>(s.length());
    double j = 0.0;
    double k = 0.0;
    int iInARow = 0;
    int sInARow = 0;
    boolean hasDecimal = false;
    boolean out = false;
    for(int i = 0 ; i < s.length() ; i++) {
        if(out)
            break;
        k = j;
        try {
            j = Integer.parseInt(s.charAt(i) + "");
            iInARow++;
            sInARow = 0;
            String d = hasDecimal == true ? "." : "";
            hasDecimal = false;
            if(iInARow >= 2) {
                Double dou = k;
                Character character = dou.toString().charAt(dou.toString().lastIndexOf(".") + 1);
                if(!character.equals('0'))
                    j = Double.parseDouble(k + d + (int) j);
                else
                    j = Double.parseDouble((int) k + d + (int) j);
            }
            d = "";
            try {
                Integer.parseInt(s.charAt(i + 1) + "");
            } catch(Exception e) {
                Character c;
                try {
                    c = s.charAt(i + 1);
                } catch(StringIndexOutOfBoundsException exc) {
                    c = 'a';
                }
                if(!c.equals('.'))
                    o.add(j);
            }
        } catch(Exception e) {
            Character c = s.charAt(i);
            if(c.equals('.')) {
                hasDecimal = true;
                continue;
            } else if(c.equals('!')) {
                o.add(0.0);
            } else if(c.toString().equals(" ")) {
                continue;
            } else {
                sInARow++;
                iInARow = 0;
            }
            if(sInARow == 2) {
                throw exception;
            } else {
                a = c.toString();
            }
            z.add(a);
            if(a.equals("!")) {
                try {
                    s.charAt(i + 1);
                } catch(StringIndexOutOfBoundsException exc) {
                    out = true;
                }
            }
        }
    }
    o.trimToSize();
    z.trimToSize();
    for(int i = 0 ; i <= o.size() ; i++) {
        if(z.contains("^")) {
            if(o.get(z.indexOf("^") + 1).toString().contains(".") && !o.get(z.indexOf("^")).toString().matches("[0-9]+\\.0"))
                throw exception;
            o.set(z.lastIndexOf("^"), power(o.get(z.lastIndexOf("^")), (int) ((o.get(z.lastIndexOf("^") + 1)) + 0)));
            o.remove(z.lastIndexOf("^") + 1);
            z.remove("^");
        } else if(z.contains("!")) {
            if(o.get(z.indexOf("!")).toString().contains(".") && !o.get(z.indexOf("!")).toString().matches("[0-9]+\\.0"))
                throw exception;
            o.set(z.indexOf("!"), (double) factorial((int) (o.get(z.indexOf("!")) + 0)));
            o.remove(z.indexOf("!") + 1);
            z.remove("!");
        } else if(z.contains("/")) {
            BigDecimal bd = new BigDecimal(o.get(z.indexOf("/")).toString());
            BigDecimal bd2 = new BigDecimal(o.get(z.indexOf("/") + 1).toString());
            bd = bd.divide(bd2);
            o.set(z.indexOf("/"), o.get(z.indexOf("/")) / o.get(z.indexOf("/") + 1));
            o.remove(z.indexOf("/") + 1);
            z.remove("/");
        } else if(z.contains("*")) {
            BigDecimal bd = new BigDecimal(o.get(z.indexOf("*")).toString());
            BigDecimal bd2 = new BigDecimal(o.get(z.indexOf("*") + 1).toString());
            bd = bd.multiply(bd2);
            bd.setScale(bd.scale() + bd2.scale());
            o.set(z.indexOf("*"), bd.doubleValue());
            o.remove(z.indexOf("*") + 1);
            z.remove("*");
        } else if(z.contains("-")) {
            o.set(z.indexOf("-"), o.get(z.indexOf("-")) - o.get(z.indexOf("-") + 1));
            o.remove(z.indexOf("-") + 1);
            z.remove("-");
        } else if(z.contains("+")) {
            o.set(z.indexOf("+"), o.get(z.indexOf("+")) + o.get(z.indexOf("+") + 1));
            o.remove(z.indexOf("+") + 1);
            z.remove("+");
        } else {
            if(!z.isEmpty())
                throw exception;
        }
    }
    return o.get(0);
}

public static int factorial(int i) {
    if(i == 1)
        return 1;
    else
        return i * factorial(i - 1);
}

public static double power(double i, int j) {
    if(j == 1) {
        return i;
    } else if(j == 0) {
        return 1;
    } else {
        return i * power(i , j - 1);
    }
}

The class name and the import statements do not matter.

Sample output:

Welcome to the Calculator!

Type a calculation:
2 + 2     <--- Input

The answer is: 4.0

Again?
yes     <--- Input

Type a calculation:
2 - 2     <--- Input

The answer is: 0.0

Again?
y     <--- Input

Type a calculation:
2 * 2     <--- Input

The answer is: 4.0

Again?
true     <--- Input

Type a calculation:
2 / 2     <--- Input

The answer is: 1.0

Again?
yep     <--- Input

Again?
yes     <--- Input

Type a calculation:
2!     <--- Input

The answer is: 2.0

Again?
t     <--- Input

Type a calculation:
2 ^ 2     <--- Input

The answer is: 4.0

Again?
no     <--- Input

Answer 1

Don't catch Exception. calculate() explicitly states that it throws IllegalArgumentException, so you should be catching that. You always want to catch the most specific exception type you can.

Don't create the exception unless you know it will be thrown. Most of the time the arguments will be valid and you will create an instance that is never referenced again.

---

if(out)
    break;

1. Always including braces will ensure that you don't accidentally have a line execute unconditionally, when you meant for it to be part of the if clause. This case is simple, but as the code gets more complicated and more deeply nested, it is easier to have mistakes.

2. out is only ever set in one place. You can remove this variable and break the loop immediately.

---

The majority of you code is dealing with numbers, stop operating on their String values.

Character character = dou.toString().charAt(dou.toString().lastIndexOf(".") + 1);
if(!character.equals('0'))
  //...

Here it looks like you are checking to see if dou has a fractional part. However, if the value is 1.02, this code will not detect the factional part.

Instead this can be done with just math.

if (dou - dou.longValue() == 0) {
  // whole number
} else {
  // has fractional part
}

Also, don't use boxed values unless you have to. In this case, I suspect it was to get access to toString(), but you can use the static Double.toString().

Looking back at the code:

if(!character.equals('0'))
    j = Double.parseDouble(k + d + (int) j);
else
    j = Double.parseDouble((int) k + d + (int) j);

I have no idea what operation you are actually doing in the if and else blocks following this check. There is nothing to indicate why some values are casted and the others are not, or why these values are being summed in the first place. This is exactly where a well named function would make the code more readable and clear.

---

j = Integer.parseInt(s.charAt(i) + "");

This is a dirty hack to create a String. All of the classes that correspond to the value types have a static toString() that you can use and will clearly state what you are doing. There is no reason to do string concatenation when the language gives you a function that does exactly what you want.

---

String d = hasDecimal == true ? "." : "";

hasDecimal is already boolean, so you don't need to do the comparison.

String d = hasDecimal ? "." : "";

---

} else if(z.contains("-")) {
    o.set(z.indexOf("-"), o.get(z.indexOf("-")) - o.get(z.indexOf("-") + 1));
    o.remove(z.indexOf("-") + 1);
    z.remove("-");
} 

First we check if "-" exists in the list. Then you get the index of "-" 4 different times. Then you remove the value. Each of these calls have to iterate over the list looking for the value. Instead, you can make one call to indexOf(), store it in a variable, and use the variable for each of the operations. That changes the process from 5 iterations to just one.

---

The only difference between the operation I just talked about with "-" and the following else if is the value it looks for. Write one function that performs this operation and then call it once with "-" and once with "+".

---

} else {
    if(!z.isEmpty())
        throw exception;
}

This is the last block in your long if else chain. Moving the if into the else block provides no benefit has it does not effect anything else if it is left as an else if.

---

Math.pow() exists, so you don't have to implement power() yourself.

Answer 2

It started off pretty well: main and doAgain are very reasonably sized.

calculate is a monster though. It's so crippled I don't even want to imagine what maintaining that code would be. Reviewing reading it is hard, for several reasons:

• Naming is simply awful.

  String a = "";
  ArrayList<Double> o = new ArrayList<Double>(s.length());
  ArrayList<String> z = new ArrayList<String>(s.length());
  double j = 0.0;
  double k = 0.0;
  int iInARow = 0;
  int sInARow = 0;
  

  The human brain my poor little fried brain isn't very good at keeping track of the identifier-meaning mappings many single-letter variables. It's much easier when the identifiers themselves are descriprive enough to convey their meaning.

  I see only one such variable:

  boolean hasDecimal = false;
  

• You have several methods to extract out of calculate - one for each operation you want to support. Don't cram everything into a calculate function.

• Exception handling is pretty tangled, hard to follow. You have a try/catch block inside a catch block, that seems to be chained (hard to tell because of indentation).

I'll leave the rest to other reviewers more familiar with java :)