Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am a beginner in Java programming. Here is my simple calculator. I hope you could scan through my coding and let me know if there is any error in my coding or how I could simplify the code.

import java.util.Scanner;

public class Decimal {
    /**
     * @param args
     */
    public static void main(String[] args) {
        double n1, n2;
        String operation;
        Scanner scannerObject = new Scanner(System.in);

        System.out.println("Enter first number");
        n1 = scannerObject. nextDouble();

        System.out.println("Enter second number");
        n2 = scannerObject. nextDouble();

        Scanner op = new Scanner(System.in);
        System.out.println("Enter your operation");
        operation = op.next();

        switch (operation)  {
        case "+":
            System.out.println("Your answer is " + (n1 + n2));
            break;

        case "-":
            System.out.println("Your answer is " + (n1 - n2));
            break;

        case "/":
            System.out.println("Your answer is " + (n1 / n2));
            break;

        case "*":
            System.out.println("Your asnwer is " + (n1 * n2));
            break;

        default:
            System.out.println("Je ne sais pas");

        }
    }
}
share|improve this question
    
Rewriting in Scala would be one way to simplify your code :) –  Jus12 Nov 6 at 6:47

3 Answers 3

Looks good for a beginner.

Couple tweaks if you want to make it look nicer. The name of the class should be somewhat describing the purpose so in your case Calculator would fit better. If you are familiar with enums you could parse your +,-,/ into a enum and do switch on that. Then next tweek could be to do the System.printout at the end and in the switch case to do just the operation.

Another suggestion would be to make 4 classes implementing binary operations with just one method taking two arguments and returning the result. The you can have a map with a key being the operation (in enum or string) and the value would be the appropriate class. then you end up with more classes but this main method gets shorter.

Generally it is better to split the code in more methods and classes and keep the motto "one class one responsibility". So as an example each of your arithmetic classes would be responsible for a single arithmetic operation. If you want to follow that completely, then you would have to create a class that gets the input from the user as well. The calculator class then would be just like a coordinator saying - give me numbers, give me operation, do the operation and printing out the result at the end.

If you want let me some comment here and I can drop you some sample code here.

share|improve this answer

This is a really neat program. For what it does, it does a good job.

Really, there's only one criticism, and a few suggestions. The criticism is designed to get you in to the right habits, it's not a major bug....

Using try-with-resources

Java, for years, has had a problem with people being lazy about closing resources.... you open a file, read it's contents, and move on.... leaving an open file handle and other resources until the Garbage Collector cleans up the mess. These 'forgotten' resources can sometimes lead to unexpected bugs and deadlocks.

In Java7, the concept 'try-with-resources' was added to ensure that resources are guaranteed to be closed nicely whether the code succeeds or not. It actually allows the programmer to be even lazier than before, and get a an even better result.

In your case, the Scanner instance is a resource that should be closed....

public static void main(String[] args) {
    try (Scanner scannerObject = new Scanner(System.in)) {
        double n1, n2;
        String operation;

        .....

    }
}

OK, that's the right way to use the Scanner.

Further Improvements

As your program expands, you will find that there are a few things that become 'uncomfortable'.... the number of operators will increase, and the complexity of the calculations will increase as well... eventually you will want to enter expressions like 100 * ( 17 / 20 ) to get the percent score if you got 17 out of 20 in a test.... etc.

What you have at the moment is great for it's purpose, but I encourage you to try to find ways to expand on the idea.

Research you may want to do is in to things like 'Reverse Polish Notation (RPN)' which is an easier-to-parse format for writing mathematical expressions. Then you can look at things that help you convert Infix to Postfix (RPN) notation...

In order to do these more expansive operations you will need to find ways to turn operators in to classes (probably an Enum), and have classes that allow you to encapsulate an expression (unary or binary expressions).

Good job on this, otherwise.

share|improve this answer

Not bad for a beginner.

You shouldn't have to instantiate more than one scanner, as your numbers and the operation are coming from the same input stream. You can even accept your input in a more human-friendly way, as one expression.

There's no need to name your scanner somethingObject — most of your variables will be objects!

You have a typo ("Your asnwer is "), which illustrates the problem with repetitive code. I'll leave it as an exercise for you to figure out a way to restructure your program to eliminate the repetitive System.out.println() calls.

I'd rename the class to something more descriptive.

public class Calculator {
    public static void main(String[] args) {
        try (Scanner scanner = new Scanner(System.in)) {
            System.out.println("Enter an expression of the form 3 * 5");
            double n1 = scanner.nextDouble();
            String operation = scanner.next();
            double n2 = scanner.nextDouble();

            switch (operation)  {
            case "+":
                System.out.println("Your answer is " + (n1 + n2));
                break;

            case "-":
                System.out.println("Your answer is " + (n1 - n2));
                break;

            case "/":
                System.out.println("Your answer is " + (n1 / n2));
                break;

            case "*":
                System.out.println("Your asnwer is " + (n1 * n2));
                break;

            default:
                System.out.println("Je ne sais pas");

            }
        }
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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