4
\$\begingroup\$

I'm making a calculator that can solve calculations that are inputed in a very raw form. Say: "3*52+11+3/2" I'm using the console, at least for now.

This will also take in consideration that multiplications and divisions should be completed first, so 2+3*4 is 14 instead of 20.

I wanted someone to see if there are more effective ways of doing what I'm doing, if I'm not making any big flaws or there are any problems in the code. This code also assumes the user enters valid text, I will latter make it so that it checks everything properly.

I tried to document everything as well as I could so it doesn't take too much to understand the logic.

Note: I know my code crashes when I input a negative number as in -2+3 and doesn't support decimal numbers in the input just yet. Any help in implementing those would be useful.

package com.fowlron.calculator;

import java.util.ArrayList;
import java.util.Scanner;

public class Calculator {
    private static ArrayList<Term> list = new ArrayList<Term>();

    public static void main(String args[]) {
        // Initial Info
        System.out.println("+ for sum");
        System.out.println("- for subtraction");
        System.out.println("* for multiplication");
        System.out.println("/ for division");
        System.out.println("E.g. 2+5*3 outputs 17");
        System.out.println("Input a calculation");

        // Opening scanner, getting input and closing it
        Scanner scan = new Scanner(System.in);
        String s = scan.nextLine();
        scan.close();

        // Looping through all chars in the input
        for (char c : s.toCharArray()) {
            if (isCharNumber(c)) {
                // Checking if the last character added was also a number
                // Also checking if size > 0 to avoid ArrayIndexOutOfBounds
                if (list.size() > 0
                        && list.get(list.size() - 1).getType()
                                .equals(Term.Type.DOUBLE)) {
                    // If so multiply by ten and add the new one
                    list.get(list.size() - 1).setNum(
                            list.get(list.size() - 1).getNum() * 10
                                    + Character.getNumericValue(c));
                } else {
                    // Otherwise, just add it regularly
                    list.add(new Term(Character.getNumericValue(c)));
                }
            } else {
                // If it's not a number, it must be an operator, so just add it
                // as a String
                // Also check for whitespaces
                if (!Character.isWhitespace(c)) {
                    list.add(new Term(Character.toString(c)));
                }
            }
        }

        // Calculating and outputting
        double result = calculate(list);
        System.out.println("The result is " + result);
    }

    private static boolean isCharNumber(char c) {
        // Thought that since it is very long I might want to put this into it's
        // own method
        if (c == '0' || c == '1' || c == '2' || c == '3' || c == '4'
                || c == '5' || c == '6' || c == '7' || c == '8' || c == '9') {
            return true;
        } else {
            return false;
        }
    }

    private static double calculate(ArrayList<Term> l) {
        // Creating a counter and a continue boolean
        int c = 0;
        boolean cont = true;

        // This loop is used twice because multiplications and divisions
        // have priority over sums and subtractions, so I need to first
        // solve all the multiplications and divisions, and only then the
        // others.
        // For example, 2+3*4 = 2+12 = 14
        // NOT 2+3*4 = 5*4 = 20

        while (cont) {
            // Checking if looped through all the elements
            if (c >= l.size()) {
                break;
            }

            // Checking if the current element is an operation
            if (l.get(c).getType().equals(Term.Type.STRING)) {
                // If so, check which it is, apply it to the previous element
                // and remove the next one followed by the current one
                Term p = l.get(c - 1);
                Term n = l.get(c + 1);
                switch (l.get(c).getOp()) {
                case "*":
                    p.setNum(p.getNum() * n.getNum());
                    l.remove(c + 1);
                    l.remove(c);
                    c--;
                    break;
                case "/":
                    p.setNum(p.getNum() / n.getNum());
                    l.remove(c + 1);
                    l.remove(c);
                    c--;
                    break;
                }
            }
            // Increasing the counter
            c++;
        }

        // Reseting the counter
        c = 0;
        while (true) {
            // Same logic here
            if (c >= l.size()) {
                break;
            }

            // Exactly the same only just for the sums and subtractions
            if (l.get(c).getType().equals(Term.Type.STRING)) {
                Term p = l.get(c - 1);
                Term n = l.get(c + 1);
                switch (l.get(c).getOp()) {
                case "+":
                    l.get(c - 1).setNum(p.getNum() + n.getNum());
                    break;
                case "-":
                    l.get(c - 1).setNum(p.getNum() - n.getNum());
                    break;
                }
                l.remove(c + 1);
                l.remove(c);
                c--;
            }
            // Increasing the counter
            c++;
        }

        // Returning the correct result
        return l.get(0).getNum();
    }
}

The term class:

package com.fowlron.calculator;

import java.util.ArrayList;

public class Term {
    // I didn't know what to call this class, specially because I'm not english
    // and didn't know a better name for it. It basically is a class that can
    // hold eigter a String, a Double, or an ArrayList of itself.
    // I make an ArrayList of these, so that I can then loop through it and
    // solve the calculation. The fact it can hold an ArrayList of itself is so
    // that when I implement things like "2*(2+3)" I can store 2+3 as another
    // list, inside the main one. The I'd first find all the lists in the main
    // list, and solve them. I might make the calculate method in the other
    // class recursive for it to call itself, which in my head seems to work,
    // and would help for parenthesis inside parenthesis.
    private Type type;
    private double i;
    private String s;
    private ArrayList<Term> al;

    public Term(double i) {
        this.i = i;
        this.s = null;
        this.al = null;
        this.type = Type.DOUBLE;
    }

    public Term(String s) {
        this.i = -1d;
        this.s = s;
        this.al = null;
        this.type = Type.STRING;
    }

    public Term(ArrayList<Term> al) {
        this.i = -1d;
        this.s = null;
        this.al = al;
        this.type = Type.ARRAYLIST;
    }

    public String getOp() {
        return s;
    }

    public double getNum() {
        return i;
    }

    public ArrayList<Term> getPar() {
        return al;
    }

    public void setOp(String s) {
        this.i = -1d;
        this.s = s;
        this.al = null;
        this.type = Type.STRING;
    }

    public void setNum(double i) {
        this.i = i;
        this.s = null;
        this.al = null;
        this.type = Type.DOUBLE;
    }

    public void setPar(ArrayList<Term> al) {
        this.i = -1d;
        this.s = null;
        this.al = al;
        this.type = Type.ARRAYLIST;
    }

    public Type getType() {
        return type;
    }

    public enum Type {
        DOUBLE, STRING, ARRAYLIST
    }
}
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Welcome to Code Review! Don't worry about the code length — we regularly review much longer programs here. \$\endgroup\$ Jan 14 '15 at 22:35
  • \$\begingroup\$ Thank you! Glad to know that this isn't too long, because with so much to implement still, since this is about only an hour of work, this is bound to be bigger, haha \$\endgroup\$
    – Fowlron
    Jan 14 '15 at 22:44
1
\$\begingroup\$

It would be better to move the input parsing logic out of Calculator.main to its own method. It would be even better to further decompose it to its elements: print intro, read input, parse input, calculate, print result.

It seems the list variable is only used in the parsing logic, so it shouldn't be a static variable of the class, but a local variable in the parsing logic.

In the parsing logic, you make many references to the last term added with list.get(list.size() - 1), which is quite tedious. It seems a Stack could be helpful here. Stacks are often used to build up and evaluate complex expressions in calculators, so I think the concept can help you in many ways.

As for the Term class, it's not so good to hold either of 3 different kind of things. It would be better to generalize to hold just one kind: a list of Term objects. You could create sub-classes of Term: Number, Operator. This is just off the top of my head, there are probably even better ways.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for everything, I thought about the list variable issue after I wrote this post. I agree that I should move the input parsing out of the main method too. I've got to look into stacks as I've never used them, thanks for the information. What troubles me is the term class problem. I get that I should have subclasses of it, however, wouldn't that require me to be calling instanceof all the time, and casting things? Is that good practice? \$\endgroup\$
    – Fowlron
    Jan 15 '15 at 21:58
  • \$\begingroup\$ Using instanceof and casting is not a good practice. Look for a way without resorting to those. \$\endgroup\$
    – janos
    Jan 15 '15 at 22:16
1
\$\begingroup\$

I'll just review the smaller parts of the code first, will dive into the logic if I have the time later...

  • The method isCharNumber() is redundant as there already exists Character.isDigit().
  • Variables declaration should be typed as the interfaces rather than a concrete class, e.g. List<Term> list = new ArrayList<>().
\$\endgroup\$
1
  • \$\begingroup\$ Character.is Digit() checks only against unicode digits though. Thought that that couold cause trouble \$\endgroup\$
    – Fowlron
    Jan 15 '15 at 7:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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