15
\$\begingroup\$

I have made a basic calculator using methods. This is my first attempt at using methods and would like to see if I can improve on this as there is a lot of repeated code.

import java.util.Scanner;


public class Calculator {


    public static void main(String[] args) {

        Scanner kb = new Scanner(System.in);

        System.out.println("Simple Calculator");

        System.out.println("\nHere are your options:");
        System.out.println("\n1. Addition");
        System.out.println("2. Subtraction");
        System.out.println("3. Division");
        System.out.println("4. Multiplication");

        System.out.print("\nWhat would you like to do?: ");
        int choice = kb.nextInt();
        System.out.println();


        if (choice == 1){
            addition();
        }
        else if (choice == 2){
            subtraction();
        }
        else if (choice == 3){
            division();
        }
        else if (choice == 4){
            multiplication();
        }

        System.out.println();
        kb.close();
    }

    public static void addition(){

        int nOne, nTwo;
        Scanner kb = new Scanner(System.in);

        System.out.println("Addition");

        System.out.print("\nFirst Number: ");
        nOne = kb.nextInt();

        System.out.print("\nSecond Number: ");
        nTwo = kb.nextInt();

        kb.close();
        System.out.println("\nSum: " + nOne + " + " + nTwo + " = " + (nOne + nTwo));
    }

    public static void subtraction(){
        int nOne, nTwo;
        Scanner kb = new Scanner(System.in);

        System.out.println("Subtraction");

        System.out.print("\nFirst Number: ");
        nOne = kb.nextInt();

        System.out.print("\nSecond Number: ");
        nTwo = kb.nextInt();

        kb.close();
        System.out.println("\nSum: " + nOne + " - " + nTwo + " = " + (nOne - nTwo));
    }

    public static void division(){
        int nOne, nTwo;
        Scanner kb = new Scanner(System.in);

        System.out.println("Division");

        System.out.print("\nFirst Number: ");
        nOne = kb.nextInt();

        System.out.print("\nSecond Number: ");
        nTwo = kb.nextInt();

        kb.close();
        System.out.println("\nSum: " + nOne + " / " + nTwo + " = " + (nOne / nTwo));
    }

    public static void multiplication(){
        int nOne, nTwo;
        Scanner kb = new Scanner(System.in);

        System.out.println("Multiplication");

        System.out.print("\nFirst Number: ");
        nOne = kb.nextInt();

        System.out.print("\nSecond Number: ");
        nTwo = kb.nextInt();

        kb.close();
        System.out.println("\nSum: " + nOne + " x " + nTwo + " = " + (nOne * nTwo));
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ If you are wondering why you post hasn't received many votes, it's because you posted your answer late in the day; no one has any ammo left. ;) \$\endgroup\$
    – syb0rg
    Apr 12, 2014 at 23:50
  • 1
    \$\begingroup\$ @syb0rg Ha! its ok :) Everyone works hard enough here and I'm sure It will get seen to in time. \$\endgroup\$ Apr 13, 2014 at 0:17

2 Answers 2

13
\$\begingroup\$

Nice job! I hope you find the comments below useful as you progress.

  1. Variable names: Try to have variable names which describe what the variable actually represents or holds. For example, I have no idea why you named your Scanner "kb".
  2. Method names: Method names should always be verb phrases (actions) which describe what the method does. So for example, your addition() method might be better named performAddition() or some such. Even just add() might work, though that might be a bit misleading and lead someone reading your code to believe that all the method does is add two numbers, when really the core functionality is user interface.
  3. Redundancy: Any time you are copy-pasting code, alarm bells should go off in your head. You do this in multiple places. Most notable is:

    System.out.print("\nFirst Number: ");
    nOne = kb.nextInt();
    
    System.out.print("\nSecond Number: ");
    nTwo = kb.nextInt();
    

What that means is that this is a prime candidate for encapsulation into its own method. Something like this:

private static int[] getNumbers() {
    int[] numbers = new int[2];
    System.out.print("\nFirst Number: ")
    numbers[0] = scanner.nextInt();
    System.out.print("\nSecond Number: ");
    numbers[1] = scanner.nextInt();
    return numbers;
}

Now we can simply call this method whenever we need to get numbers from the user. Much easier, and we've reduced the complexity of our code.

Another thing that's redundant is your constant re-declaration of kb = new Scanner(System.in). You really only need to do this once. If it were me, I'd have it as a static field for the entire class, e.g.,

private static final Scanner STDIN = new Scanner(System.in);

This is the basic rule for how constants are defined in Java. I named the variable STDIN because this is a convention for referring to the "standard input" stream, but SCANNER or something would be just as good.


Some other small things

You should consider using a switch statement on your choice variable:

switch(choice) {
case 1:
    performAddition();
    break;
case 2:
    performSubtraction();
    break;
case 3:
    performDivision();
    break;
case 4:
    performMultiplication();
    break;
default:
    System.out.println("Invalid choice!");
    break;
}

... or you could write it all in-line, if it looks better to you, since each block only really does one thing:

switch(choice) {
case 1: performAddition(); break;
case 2: performSubtraction(); break;
// etc.
}

This would also be a perfect use-case for enums, but that my be getting a bit too complicated for a starting project.


Small final note: I think you have a bug. The line where you display the results always says "sum", even when you're doing division, multiplication, or subtraction. :)

\$\endgroup\$
2
  • \$\begingroup\$ pastebin.com/70wnzARQ - Thanks mate, I think this new version covers most things you mentioned :) \$\endgroup\$ Apr 13, 2014 at 21:17
  • \$\begingroup\$ I look forward to seeing new code on this program. \$\endgroup\$
    – Malachi
    Apr 14, 2014 at 15:41
6
\$\begingroup\$

Your flow is a little clunky.

If I were going to calculate something I would do it in this order:

Number1 then operator then Number2

This way I know what I am doing, especially when playing with division.

I think you should ask for the first number then ask for the operation and then ask for the third number.

Also change your choice to take input of +, -, *, / as well as the numbered choices, in doing this you are one step closer to taking this to a new level with forms or whatever direction your application is going to take away from the console.

Eventually you can add something to this so that it distinguishes the operators separately and the user can write an expression rather than answer questions about the expression, I think that is really the goal right?

This sounds like a fun project!

\$\endgroup\$
2
  • 1
    \$\begingroup\$ These are really good tips if you plan to expand this calculator project in the future to continue learning. \$\endgroup\$
    – asteri
    Apr 14, 2014 at 15:52
  • \$\begingroup\$ I plan on trying to create my own calculator when I get home today. \$\endgroup\$
    – Malachi
    Apr 14, 2014 at 15:55

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