5
\$\begingroup\$

I wrote this code for a simple calculator. It creates a java file from the term, compiles and executes it. Now my question is : Is there a better way of doing this ? Would it be more efficient to analyze the string and then evaluate it step by step in the program ? Is this the right way of doing java ? Do I have to improve my 'style' ?

class Calculator {
    public static double evaluate(String term) throws IOException {
        String result=term;
        char[] n={'0','1','2','3','4','5','6','7','8','9','+','-','*','/','(',')'};
        boolean number=false;
        boolean add=true;
        int adds=0;
        for (int i=0; i < term.length(); i++) {
            number=false;
            for (int i2=0; i2 < n.length; i2++) {
                if (term.charAt(i)==n[i2]) {
                    number=true;
                    add=true;
                    break;
                } else {
                }
            }
            if (number==false && add==true)  {
                String p1=result.substring(i+adds*5,result.length());
                String p2=result.substring(0,i+adds*5);
                result=p2+"Math."+p1;
                adds++;
                add=false;
            }
        }
        JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
        FileOutputStream terminus = new FileOutputStream("Term.java");
        terminus.write(new String("import java.lang.Math; import java.io.FileOutputStream; import java.io.IOException; public class Term { public static void main(String[] args) {try {FileOutputStream result = new FileOutputStream(\"result.txt\");result.write(Double.toString("+result+").getBytes());} catch (Exception e) {}}}").getBytes());
        compiler.run(System.in, System.out, System.err, "Term.java");
        Runtime.getRuntime().exec("java Term");
        int i=0;
        FileInputStream resu = new FileInputStream("result.txt");
        String r="";
        i=resu.read();
        while(i != -1) {
            r=r+new String(new char[] {(char)(i)});
            i=resu.read();
        }
        return Double.parseDouble(r);
    }
}

And here's how I use it :

          try {
          System.out.println(Calculator.evaluate("cos(1+1)*sin(7+9)/atan(7)"));
          }
          catch (Exception e) {}
\$\endgroup\$
  • 1
    \$\begingroup\$ Why would you make a calculator this way instead of making it a normal calculator with RPN and stacks or something similar? This is kind of trying to emulate eval in Python in Java. Not a good idea, IMHO. And you shouldn't be needing that try-catch in calling code, wrap the IOException in a RuntimeException instead to make it unchecked. \$\endgroup\$ – Tamoghna Chowdhury Mar 11 '17 at 10:30
6
\$\begingroup\$

Let me answer your questions first:

  1. Yes, there is a better way of doing this, and you already know it as you have suggested it yourself.

  2. Yes, it would be more efficient to do it manually by analyzing the String and evaluating it part-by-part, like an interpreter would.

  3. This is most definitely the wrong way of doing Java (not mincing my words here, you know that you're doing it wrong, otherwise that question wouldn't have occurred to you).

  4. Yes, you have to improve your style. See below.

My Views:

Your approach to a calculator is certainly novel, for Java. This is highly inefficient (you have to spawn new processes, which is always slow), as you have suspected. What you do is basically emulating eval of Python. Bad idea, but you've done it. Now how to go forward? (Yes, doing this the "classic" way is the way to do it in Java. Doing it this way feels like you are forcing Java to be more Python-ish.)

Style:

  1. Space around operators, please! Whitespace is free and makes the code that much more readable.

  2. Magic numbers. Instead of 5 everywhere, put

    private static String prefix = "Math.";
    private static int prefixLength = prefix.length();
    

    at the top of the Calculator class, and use those instead of their values. More self-explanatory and much easier to read and maintain. This prefix business is voided by another suggestion below, though.

  3. Weird variable names. Nested loop indices are usually named i,j,k, etc., like in mathematics. What do adds and add even mean? p1 and p2 are as good as foo and bar in telling what they contain, which is no good.

Code:

  1. Why do you wrap a String literal into a String object using a String(String) constructor? (If that sounded weird to you, better drop the new String("...") for just "..." everywhere.

  2. Where you are creating your Term.java, you don't need the import java.lang.Math, it's imported by default as it is part of the java.lang package.

  3. What you might want to do is have import static java.lang.Math.*, which is a better idea as it allows you to skip all the weird string processing going on in the loop where you prepend "Math." wherever there is a function call in the term.

  4. Why an empty else? If you don't need it, skip it.

  5. Comparing boolean values against boolean literals is not required. For <x> == true, use just <x>, and for <x> == false, use just !<x>

  6. Wrap the possible IOException in a RuntimeException and rethrow it. This prevents the caller from having to rethrow the exception or gobble it (like you do) compulsorily.

  7. Try FileReader and FileWriter, especially wrapped up in a BufferedReader and BufferedWriter, respectively, instead of using FileInputStream and FileOutputStream raw - the former ones will allow you to read and write whole Strings at a time, getting rid of all the getBytes() calls you have on Strings (horrible idea especially if you don't specify your file encodings)

  8. String.format(String, Object...) is a good idea if you have to create a String with variables interpolated into it.

\$\endgroup\$
  • \$\begingroup\$ Nice review ! Ill try to make it better next time... \$\endgroup\$ – LMD Mar 11 '17 at 14:38

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.