2
\$\begingroup\$

I wrote a simple calculator which uses orders of operations. I would like to know, do you see any serious flaws in logic and what do you think about this solution? It is my second approach to a problem, and the code passes tests (with integers and decimals) for basic operations: ^,|(i used this sign for root square), *, /, +, -.

import java.math.*; import java.util.*; public class OrderOfOperations { ArrayList<String> contents; String item; OrderOfOperations check; public static void main (String[] args){ Scanner input = new Scanner(System.in); System.out.println("Enter an operation: "); String a = input.nextLine(); OrderOfOperations go = new OrderOfOperations(); a = go.brackets(a); System.out.println("Result: "+a); } public String brackets(String s){ //method which deal with brackets separately check = new OrderOfOperations(); while(s.contains(Character.toString('('))||s.contains(Character.toString(')'))){ for(int o=0; o<s.length();o++){ try{ //i there is not sign if((s.charAt(o)==')' || Character.isDigit(s.charAt(o))) //between separate brackets && s.charAt(o+1)=='('){ //or number and bracket, s=s.substring(0,o+1)+"*"+(s.substring(o+1)); //it treat it as } //a multiplication }catch (Exception ignored){} //ignore out of range ex if(s.charAt(o)==')'){ //search for a closing bracket for(int i=o; i>=0;i--){ if(s.charAt(i)=='('){ //search for a opening bracket String in = s.substring(i+1,o); in = check.recognize(in); s=s.substring(0,i)+in+s.substring(o+1); i=o=0; } } } } if(s.contains(Character.toString('('))||s.contains(Character.toString(')'))|| s.contains(Character.toString('('))||s.contains(Character.toString(')'))){ System.out.println("Error: incorrect brackets placement"); return "Error: incorrect brackets placement"; } } s=check.recognize(s); return s; } public String recognize(String s){ //method divide String on numbers and operators PutIt putIt = new PutIt(); contents = new ArrayList<String>(); //holds numbers and operators item = ""; for(int i=s.length()-1;i>=0;i--){ //is scan String from right to left, if(Character.isDigit(s.charAt(i))){ //Strings are added to list, if scan finds item=s.charAt(i)+item; //a operator, or beginning of String if(i==0){ putIt.put(); } }else{ if(s.charAt(i)=='.'){ item=s.charAt(i)+item; }else if(s.charAt(i)=='-' && (i==0 || (!Character.isDigit(s.charAt(i-1))))){ item=s.charAt(i)+item; //this part should recognize putIt.put(); //negative numbers }else{ putIt.put(); //it add already formed number and item+=s.charAt(i); //operators to list putIt.put(); //as separate Strings if(s.charAt(i)=='|'){ //add empty String to list, before "|" sign, item+=" "; //to avoid removing of any meaningful String putIt.put(); //in last part of result method } } } } contents = putIt.result(contents, "^", "|"); //check Strings contents = putIt.result(contents, "*", "/"); //for chosen contents = putIt.result(contents, "+", "-"); //operators return contents.get(0); } public class PutIt{ public void put(){ if(!item.equals("")){ contents.add(0,item); item=""; } } public ArrayList<String>result(ArrayList<String> arrayList, String op1, String op2){ int scale = 10; //controls BigDecimal decimal point accuracy BigDecimal result = new BigDecimal(0); for(int c = 0; c<arrayList.size();c++){ if(arrayList.get(c).equals(op1)|| arrayList.get(c).equals(op2)){ if(arrayList.get(c).equals("^")){ result = new BigDecimal(arrayList.get(c-1)).pow(Integer.parseInt(arrayList.get(c+1))); }else if(arrayList.get(c).equals("|")){ result = new BigDecimal(Math.sqrt(Double.parseDouble(arrayList.get(c+1)))); }else if(arrayList.get(c).equals("*")){ result = new BigDecimal(arrayList.get(c-1)).multiply (new BigDecimal(arrayList.get(c+1))); }else if(arrayList.get(c).equals("/")){ result = new BigDecimal(arrayList.get(c-1)).divide (new BigDecimal(arrayList.get(c+1)),scale,BigDecimal.ROUND_DOWN); }else if(arrayList.get(c).equals("+")){ result = new BigDecimal(arrayList.get(c-1)).add(new BigDecimal(arrayList.get(c+1))); }else if(arrayList.get(c).equals("-")){ result = new BigDecimal(arrayList.get(c-1)).subtract(new BigDecimal(arrayList.get(c+1))); } try{ //in a case of to "out of range" ex arrayList.set(c, (result.setScale(scale, RoundingMode.HALF_DOWN). stripTrailingZeros().toPlainString())); arrayList.remove(c + 1); //it replace the operator with result arrayList.remove(c-1); //and remove used numbers from list }catch (Exception ignored){} }else{ continue; } c=0; //loop reset, as arrayList changed size } return arrayList; } } }

I tested code for inputs such as:

  • 1-(1+1)+1,
  • 2*(5*(8/2)),
  • |3+3.6589,
  • 9-5/(8-3)*2+6,
  • -1--1--1--1+|4^2,
  • 1+7/3*(34.67/23-(-2--5)+6^2),
  • 2(2(2(2(2(2))))),
\$\endgroup\$
2
  • \$\begingroup\$ could you give us a couple of example inputs? \$\endgroup\$
    – tim
    Mar 4, 2015 at 19:35
  • \$\begingroup\$ ok, I got it now. 5+(3*2) would be one example, and spaces are not allowed. \$\endgroup\$
    – tim
    Mar 4, 2015 at 19:44

1 Answer 1

7
\$\begingroup\$

OOP

Your internal PutIt class doesn't have any state itself, but uses the fields of the enclosing class, which is quite confusing. PutIt could just as well be removed and its methods made part of OrderOfOperations.

It's also rarely needed that a class has a field storing an instance of the class itself (linked lists would be an example where this makes sense). If I remove the check field from your code and call all the methods directly, it still seems to work perfectly.

I don't want to do a complete redesign of your code, but if you do want to use classes, classes such as Operation and Equation might be more useful.

Error Handling

Your code doesn't handle invalid input very well. As I had no idea what valid input looks like, I tried a couple of things:

  • 5 + 3 -> java.lang.NumberFormatException: I would expect at least a message of what string could not be formated (although your program should be able to handle spaces and not throw an error at all).
  • + 4 2 -> java.lang.ArrayIndexOutOfBoundsException: -1: Again, catching this and presenting a meaningful error message would be nice.

Naming

Variable names should help a reader understand your code. They should be as expressive as possible.

  • Yours are often very generic. contents of what? what item? check what? PutIt what's it? and put it where? go where?
  • short variable names are also almost never good. c, a, and s are all not very expressive. Especially bad is o as a loop variable, because it results in code like i=o=0, which is quite hard to read.
  • methods should be named after what they do. brackets eg doesn't tell me that at all (does it process brackets? does it find brackets?). Same with recognize (what does it recognize? and what does recognize even mean in this context?), put (put what where?) and result (does it print the result? does it create the result? the result of what?).

Misc

  • don't import *, but the concrete classes that you need.
  • use private or public for fields.
  • use a lot more spaces to increase readability (you can use an IDE to format this for you).
  • don't ignore exceptions. If you don't want to deal with them, throw them upwards. Just swallowing them will make it very hard to find bugs.
  • declare fields in as small a scope as possible. contents and item are both only used in recognize and put. It would be a lot better to declare them inside recognize and pass them as arguments to put.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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