11
\$\begingroup\$

This is my Java 8 code for a console calculator. I just want to know if it can be improved in any way. Just so you know i am a real beginner just got into java about a couple of weeks ago.

    import java.util.Scanner;

public class Algorithm1 {
public static int FirstNumber, SecondNumber;
public static float TheAnswer;
public static String Action;
public static Scanner ScanInt1, ScanString;
public static boolean hasString;
public static boolean hasString2, hasInt;
public static void main(String args[]){
hasString = false;//sets up the booleans
hasString = false;
hasInt = false;
ScanInt1 = new Scanner(System.in);
ScanString = new Scanner(System.in);
System.out.println("Give your First input please.");
while  (FirstNumber < 1){
FirstNumber = ScanInt1.nextInt();   
}
System.out.println("Give your Second input please.");
hasString = true;
if (SecondNumber < 1){
SecondNumber = ScanInt1.nextInt();      
}
else {
System.out.println("Now * or / or + or -");
hasString2 = true;
}
Action = ScanString.nextLine();
System.out.println(Answer());
}
public static float Answer(){
//This method is for the math
    if (Action.equals("*")){
        TheAnswer = FirstNumber * SecondNumber;

    } 
if (Action.equals("/")){
    TheAnswer = FirstNumber / SecondNumber;

}
if (Action.equals("+")){
    TheAnswer = FirstNumber + SecondNumber;

}
if (Action.equals("-")){
    TheAnswer = FirstNumber - SecondNumber;

}
return TheAnswer;
}

}
\$\endgroup\$
0

5 Answers 5

4
\$\begingroup\$

There are excellent suggestions in the above text, and I'd echo most of them.

The key element in writing better code, though, is to understand the goal of your source code.

This might be a bit above your level, but think about it this way: Why do we write source code in higher level languages?

Why does a language support upper and lower case? Why does a language have support for white space? Why does a language have support for comments?

The truth is, the java compiler doesn't care about most of those things (mixed-case aside).

The reason we write in higher level languages is to make it easier for someone else (who may be you!) to understand what was INTENDED by the software. Source code is for HUMAN BEINGS to read and write.

So my advice is to make the source code as clear, easy to read, and easy to understand as you possibly can.

As h.j.k. said above, you should camel-case method and variable names, and Pascal-case type/class names. Why? Because it's the convention that's used by programmers all over the planet. It makes it easier for other people to read software you've written, if you follow the convention/style that they're used to reading.

Same rules regarding white-space, such as indentation. Matching up braces to find the blocks that are under an if/then/else is nobody's idea of a good time. Make it easy for them to find where the blocks end.

I would differ with h.j.k. on one point, though, and that's the use of multiple return statements in your Answer() method. I can certainly see his point; his way of thinking reduces the number of lines in the method, and makes it easier to see at a glance what is being returned by the method. These are all admirable goals.

However, in non-trivial software, I much prefer to see a single return statement at the bottom of the routine, because it means that I can set a single breakpoint at the return, and I can see what the function is returning in the debugger. This is a big help when developing/maintaining software written by multiple people over multiple years.

I also prefer that the local variable being returned is initialized to some known-good initial value. This guarantees the return value from the function will always be set to SOMETHING.

That's one thing you're not doing currently. You're depending on the global variable to be zero, and not explicitly setting the TheAnswer to any value.

What happens if the user enters an operator that's not a */+-?

This is becoming long winded, so I'll try show you what I think it should look like. DO NOT TURN THIS IN AS YOUR HOMEWORK. It will be very obvious to your teacher that you didn't write it. I'm not going to fix the variable declarations and such, only attempt to show you how other programmers write software.

Trust me, there's a ton more I could do to this, but I'm not fixing any of the errors you might have.

You're asking the right questions, and I'm glad to see you want to do better.

An ancient book regarding this very topic was called "Professional Pascal" by Henry Ledgard. Don't let the Pascal in the title fool you; it's about writing professional grade software, in any language.

Professional Pascal (Google Books)

I hope this helps.

-john

/*
**  Algorithm1
**
**  A simple console-based calculator.
**
**  USAGE:  
**      java Algorithm1
**
**  Example:
**      java Algorthim1
**          Give your First input please.
**          10
**          Give your Second input please.
**          10
**          Now * or / or + or -
**          *
**          100
**
*/
import java.util.Scanner;

public class Algorithm1 
{
    public static int FirstNumber, SecondNumber;
    public static float TheAnswer;
    public static String Action;
    public static Scanner ScanInt1, ScanString;
    public static boolean hasString;
    public static boolean hasString2, hasInt;

    /*  Answer()
    **
    **  Compute the result of FirstNumber <operator> SecondNumber, and return the value.
    */
    public static float Answer()
    {
        TheAnswer = -1.0f;  /*  Initialize to some value; this should, in best practice, be a local variable. */

        //This method is for the math

        /*  Multiplication */
        if( Action.equals( "*" ) )
        {
            TheAnswer = FirstNumber * SecondNumber;
        } 

        /*  Division */
        if( Action.equals( "/" ) )
        {
            TheAnswer = FirstNumber / SecondNumber;
        }

        /*  Addition */
        if( Action.equals( "+" ) )
        {
            TheAnswer = FirstNumber + SecondNumber;
        }

        /*  Subtraction */
        if( Action.equals( "-" ) )
        {
            TheAnswer = FirstNumber - SecondNumber;
        }
        return TheAnswer;
    }

    /*  Main
    **      No arguments.
    */
    public static void main( String args[] )
    {
        hasString = false;  //sets up the booleans
        hasString = false;
        hasInt = false;
        ScanInt1 = new Scanner(System.in);
        ScanString = new Scanner(System.in);
        System.out.println("Give your First input please.");
        while  (FirstNumber < 1)
        {
            FirstNumber = ScanInt1.nextInt();   
        }
        System.out.println("Give your Second input please.");
        hasString = true;
        if (SecondNumber < 1)
        {
            SecondNumber = ScanInt1.nextInt();      
        }
        else 
        {
            System.out.println("Now * or / or + or -");
            hasString2 = true;
        }
        Action = ScanString.nextLine();
        System.out.println(Answer());
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ "in non-trivial software, I much prefer to see a single return statement at the bottom of the routine, because it means that I can set a single breakpoint at the return" I kind of get what you mean, but then I'll put the breakpoint at the start of the method, and simply step over the less important lines. In fact, for non-trivial methods, returning early is even more advantageous as you will know clearly when the method is 'done', or when it's stuck at doing something wrong. :) \$\endgroup\$
    – h.j.k.
    Commented Jul 10, 2015 at 0:17
  • \$\begingroup\$ I am confused i should i use switch statements or not? very helpful advice though \$\endgroup\$
    – user77204
    Commented Jul 10, 2015 at 8:32
  • \$\begingroup\$ switch still makes sense for a fixed set of discrete values such as int or enum values, and Strings which are by definition immutable. \$\endgroup\$
    – h.j.k.
    Commented Jul 10, 2015 at 9:13
  • 1
    \$\begingroup\$ @h.j.k. I see what you're saying, but using your method, I set a breakpoint and then press "step" "step" "step"... In fact, I do this too, but I like the breakpoint on the single return because sometimes weird stuff happens and I return unexpectedly. That single return with a breakpoint helps minimize the frustration with that. Too many returns from a function, from all kinds of different events, feels a lot like having gotos everywhere. You're never quite certain where the control flow of the code is taking you. \$\endgroup\$ Commented Jul 10, 2015 at 11:01
  • \$\begingroup\$ You mentioned that you should camelCase variable names, yet you still have variables such as FirstNumber? \$\endgroup\$
    – juunas
    Commented Jul 12, 2015 at 3:57
12
\$\begingroup\$

A CLI-based calculator is often a good way to prepare one for bigger programming tasks, as there are just so many ways of doing it, catering for beginner to advanced implementations. I happen to have one myself about two months ago, for reference. :)

Indentation

There's no other way of putting this, but your indentation is extremely poor. Nested code blocks inside { } braces should be indented accordingly, instead of left-aligning everything.

Naming style

Please use camelCase instead of PascalCase for your variable and method names. As you can tell from the syntax highlighting on your present code, PascalCase is the naming convention for type/class names. This makes reading your code slightly harder as other developers have to 're-parse' your variable/method names from what looks like class names.

try-with-resources

Since Java 7, try-with-resources is the recommended approach to read from an I/O source, such as wrapping a Scanner over System.in:

try (Scanner scanner = new Scanner(System.in)) {
    // ...
    String line = scanner.nextLine();
    // ...
}

This lets the JVM close I/O resources automatically for you.

Less reliance on 'global' static variables

You should slowly adopt better object-oriented (OO) programming practices, such as relying less on static variables. It's better to make more effective use of method arguments so that other developers can understand what the method requires. More on this will be illustrated below.

return early

Inside your Answer() method, you should return early so that you can do away with temporary variables. In my opinion, temporary variables are often a crutch to writing longer methods, which makes code harder to read and understand in the long run. A possible improvement can be:

private static float getAnswer(String action, int first, int second) {
    switch (action) {
    case "+":
        return (float) first + second;
    case "-":
        return (float) first - second;
    // ...
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ you know, some code is indented, while others isn't. This leads me to think that some code is copied from somewhere \$\endgroup\$ Commented Jul 9, 2015 at 17:23
4
\$\begingroup\$

Indentation

Auto-formatting yields:

import java.util.Scanner;

public class Algorithm1 {
    public static int FirstNumber, SecondNumber;
    public static float TheAnswer;
    public static String Action;
    public static Scanner ScanInt1, ScanString;
    public static boolean hasString;
    public static boolean hasString2, hasInt;
    public static void main(String args[]) {
        hasString = false; //sets up the booleans
        hasString = false;
        hasInt = false;
        ScanInt1 = new Scanner(System. in );
        ScanString = new Scanner(System. in );
        System.out.println("Give your First input please.");
        while (FirstNumber < 1) {
            FirstNumber = ScanInt1.nextInt();
        }
        System.out.println("Give your Second input please.");
        hasString = true;
        if (SecondNumber < 1) {
            SecondNumber = ScanInt1.nextInt();
        } else {
            System.out.println("Now * or / or + or -");
            hasString2 = true;
        }
        Action = ScanString.nextLine();
        System.out.println(Answer());
    }
    public static float Answer() {
        //This method is for the math
        if (Action.equals("*")) {
            TheAnswer = FirstNumber * SecondNumber;

        }
        if (Action.equals("/")) {
            TheAnswer = FirstNumber / SecondNumber;

        }
        if (Action.equals("+")) {
            TheAnswer = FirstNumber + SecondNumber;

        }
        if (Action.equals("-")) {
            TheAnswer = FirstNumber - SecondNumber;

        }
        return TheAnswer;
    }

}

Formatting actually shows that:

  • You defined the same variable two times:

    hasString = false; //sets up the booleans
    hasString = false;
    
  • You got an unused variable:

    hasInt = false;
    

Also please be more precise and consistent in the UI:

  • Ask for an integer, not for an 'input'
  • Display a prompt also for the operand as you did for the numbers.

I suggest factoring out the askInteger logic:

public static int askInteger(String prompt, Scanner scanner) {
    int n = 0;
    System.out.println(prompt);
    while (n < 1) {
        n = scanner.nextInt();
    }
    return n;
}

You can call it like:

public static void main(String[] args) {
    Scanner scanner = new Scanner(System.in);
    int a = askInteger("a: ", scanner);
    int b = askInteger("b: ", scanner);

    System.out.println(a);
    System.out.println(b);

}

Coupling this function and @h.j.k 's function you can have very readable code.

\$\endgroup\$
3
  • \$\begingroup\$ I have a mac and i think thats the reason why i can't auto format \$\endgroup\$
    – user77204
    Commented Jul 9, 2015 at 12:45
  • \$\begingroup\$ @TLDuys no problem, there is not even need to install anything, just use a website of your choosing: google.it/… \$\endgroup\$
    – Caridorc
    Commented Jul 9, 2015 at 12:49
  • 2
    \$\begingroup\$ @TLDuys auto formatting is a feature of your IDE/editor not your computer, \$\endgroup\$ Commented Jul 9, 2015 at 13:05
3
\$\begingroup\$

2 Scanners wrapping the same input stream will conflict with each other. They will each read a portion of the input and interpret the data and then keep "leftover" data cached for the next call to next*(). This cache is not shared between scanners.

It's better to use just the one scanner.

\$\endgroup\$
3
\$\begingroup\$

Definitely follow h.j.k's guidance here, especially concerning the variable names, they stand out as unconventional and looked like classes, at a glance.

Since this is meant to be a console calculator, you may want to consider is optionally reading and running from the arguments.

Something along the lines of:

public static void main(String[] args) {
        if (args.length == 3) {
            parse(args);
        } else {
            // run your current version
        }
    }

    public static void parse(String[] args) {
        double first = Double.parseDouble(args[0]);
        char operator = args[1].charAt(0);
        double second = Double.parseDouble(args[2]);

        System.out.println(compute(first, operator, second));
    }

    public static double compute(double num1, char operator, double num2) {
        double result = 0;

        switch(operator) {
            case '+':
                result = num1 + num2;
            break;
            case '-':
                result = num1 - num2;
            break;
            case 'x':
                result = num1 * num2;
            break;
            case '/':
                result = num1 / num2;
            break;
            case '%':
                result = num1 % num2;
            break;
        }

        return result;
    }

So you can optionally do it the input -response - output method, or simply call it with what you want to be computed:

Sample output:

Java Algorithm1 3 * 5
15.0

Java Algorithm1 120 % 14
8.0

Of course whether you want the operator to come first, want to add support for unlimited arguments, checks for division by 0 or more is up to you, and would make for a good exercise.

\$\endgroup\$
1
  • \$\begingroup\$ I would further simplify your compute function by returning directly: public static double compute(double num1, char operator, double num2) { switch(operator) { case '+': return num1 + num2; case '-': return num1 - num2; case 'x': return num1 * num2; case '/': return num1 / num2; case '%': result = num1 % num2; } } \$\endgroup\$
    – Caridorc
    Commented Jul 9, 2015 at 11:14

Your Answer

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