11
\$\begingroup\$

I've been learning Java for about 3 weeks now, and I was hoping someone could check over my code for me, and let me know how to improve.

I am aware that the maths class could be removed (I could've typed answer = inputA * inputB etc. within the switch statement) but this is just because I wanted to practice using multiple classes.

import java.util.Scanner;

public class Calculator {

public static void main(String[] args) {

    Scanner input = new Scanner(System.in);

    Maths Maths = new Maths();

    double answer = 0;
    double inputA, inputB;
    char operator;
    boolean done = false;

     while (done == false) {
        System.out.print("Please enter your sum: ");

        inputA = input.nextDouble();
        operator = input.next().charAt(0);
        inputB = input.nextDouble();        

        switch (operator) {
            case '+': answer = Maths.add(inputA, inputB);
                      break;
            case '-': answer = Maths.subtract(inputA, inputB);
                      break;
            case '*': answer = Maths.multiply(inputA, inputB);
                      break;
            case '/': answer = Maths.divide(inputA, inputB);
                      break;
            case '^': answer = Maths.power(inputA, inputB);
                      break;
        }

            System.out.println(answer);             
    }       

    input.close();

  }

}

And my second class...

public class Maths {

    double add(double a, double b) {
        double answer = a+b;
        return answer;          
    }

    double subtract(double a, double b) {
        double answer = a-b;
        return answer;          
    }

    double multiply(double a, double b) {
        double answer = a*b;
        return answer;          
    }

    double divide(double a, double b) {
        double answer = a/b;
        return answer;          
    }

    double power(double a, double b){
        double answer =a;

        for (int x=2; x<=b; x++){
            answer *= a;
        }

        return answer;
    }

}
\$\endgroup\$
  • \$\begingroup\$ rhetorical question: why does java use: public static double sqrt(double a) ? see the API docs.oracle.com/javase/7/docs/api/java/lang/…. hint: good book, effective java, 2nd edition. Sometimes methods should be static. \$\endgroup\$ – Thufir Sep 10 '13 at 11:58
13
\$\begingroup\$

For a beginner it's a good approach. You need to know about Java Naming Convention. We use mixed case for naming any variable or instance of a class. So in your code Maths Maths = new Maths(); should be Maths maths = new Maths();.

Or even better make all the methods in Maths class public static as the instance of the class doesn't play any part in computation. Point to be noted in Java we make utility class's methods static as you can see in API's Math class, Array class.

Suggestions

  • In add, subtract methods you don't need to introduce a new answer variable. Just return the result like return (a+b); or return (a-b);.
  • while (done == false) is yukkk. Just make it while (!done).

Thumbs up for closing the Scanner. Now read about Exception handling. Happy programming!!! :D

\$\endgroup\$
17
\$\begingroup\$
  • power should probably be using Math.pow(). I'm picturing how slow your code would be at calculating 1.00000015000000. Or how inaccurate it'll be when someone wants to raise to a non-integer power. If those are not intended to be allowed, then passing doubles all over the place seems odd. But it'd be better to check beforehand and throw an error than return a blatantly incorrect result, in my opinion.

  • You never set done to true, and thus have an infinite loop; take out the flag and say while (true) or for (;;), so that that's apparent. Don't add the flag back til you're also adding the code that sets it.

  • The fact that you use objects doesn't make your code object-oriented. How you use them is what makes the difference. In this case, you're using classes as namespaces for functions, which is not very object-oriented at all.

    In your case, Maths is entirely useless; everything it does would be better done in your own code. This is an example of gratuitous use of objects. If you wanted to practice your OOP, you might want to think about how you'd get rid of that switch statement. Hint:

    public interface BinaryOperation {
        public double resultFor(double left, double right);
    }
    
    
    class Multiplication implements BinaryOperation {
        public double resultFor(double left, double right) {
            return left * right;
        }
    }
    
    class Addition implements BinaryOperation {
        public double resultFor(double left, double right) {
            return left + right;
        }
    }
    
    private Map<Character, BinaryOperation> operations;
    
    ...
    
    operations.put('*', new Multiplication());
    operations.put('+', new Addition());
    
    ...
    
    BinaryOperation op = operations.get(operator);
    if (op != null) {
        answer = op.resultFor(inputA, inputB);
    }
    else {
        System.out.println("Error: Unknown operator");
    ]
    
\$\endgroup\$
  • 1
    \$\begingroup\$ Making a class for just multiplication is overkill. OP is just a beginner, he needs time to know about Map. Give him time to digest :P. \$\endgroup\$ – Anirban Nag 'tintinmj' Sep 8 '13 at 11:56
  • 5
    \$\begingroup\$ @tintinmj: He wanted to learn about OOP. OOP is about more than just saying new a lot. :P If he's not using polymorphism or encapsulating something, he's not using OOP. In such cases, he would be better off not using it just yet than using it wrong from the very start and getting the wrong ideas ingrained. \$\endgroup\$ – cHao Sep 8 '13 at 12:01
  • 1
    \$\begingroup\$ Thanks-- I'll take that on board. As you say, I don't want to get in any bad habits early on \$\endgroup\$ – Jim Sep 8 '13 at 12:05

protected by 200_success Apr 7 '14 at 5:47

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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