HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Simple calculation in Java

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
calculationsimplejava

Problem

I am kind of new to Java and I have tried making a simple calculator. This lets you do different calculations in a row until you type done then you get the answer. It works as I expected but I was just wondering if I could get some feedback on it and how I can improve on it or what better approaches I could have taken.

This is my main class:

public class Main {
static  float Result = 0.0f;
static String input, Iteration;
static float firstInput = 0.0f;
static float secondInput;
static Calculations calculations = new Calculations();

public static void main(String[] args) {
    getFirstNumber();
    tryCatchMethod();
    calculations.sortCalc();
    outPutResult();

}

public static void getFirstNumber(){
    input = getInput("Enter the first number to work with");

}

public static void getIteration(){
    Iteration = getInput("Please enter the number");
    try {
        secondInput = Float.parseFloat(Iteration);
    } catch (NumberFormatException e) {

        input = getInput("Please enter a valid number");
        tryCatchMethod();   ////recursion, this is for when a valid number is not entered. This will continue happening until a valid number is entered.
    }
}

private static void tryCatchMethod(){

    try {
        firstInput = Float.parseFloat(input);
        Result = firstInput;
    } catch (NumberFormatException e) {

        input = getInput("Please enter a valid number");
        tryCatchMethod();   ////recursion, this is for when a valid number is not entered. This will continue happening until a valid number is entered.
    }

}

public static void outPutResult(){

    System.out.println("the result is :" + Result);
}

static String getInput(String prompt) {
    BufferedReader stdin = new BufferedReader(new InputStreamReader(System.in));

    System.out.print(prompt);
    System.out.flush();

    try {
        return stdin.readLine();
    } catch (Exception e) {
        return "Error: " + e.getMessage();
    }
}

}


Then the class with my

Solution

First things first, you can compare your solution with some of the other calculator implementations in Java on this site. :)

Names in Java

The naming convention used in Java is camelCase for field names, and preferably the singular form for class names. The preference is such that you have instance-s of a Customer class, not Customers.

You have an inconsistent approach to your field names (input, Iteration), and since you are getting started in Java, I will suggest sticking to the naming convention so tha it's easier for you to read and understand other Java code using the same convention, in the long run.

Class inheritance

You may have misunderstood the usage of class inheritance given the extent of having two classes (Multiplication, Calculations) extending another (Main) when they don't share a common... state.

First, your Multiplication class name is ill-advised. When I first looked at it, I thought you were going to create sub-classes for all four arithmetic operators. It turns out that Multiplication implements them as four methods. Furthermore, the only relation between this class and the parent class Main is the sharing of a single static field Result (I'll get to this later).

The usual approach for this kind of functionality is to re-think how your arithmetic methods take in two inputs to give you an output. Setting aside the float-or-double question, you should consider having methods like this:

public static float plus(float a, float b) {
    return a + b;
}


  • Method visibility modifier is public as it is likely to be used outside the class, not because everything defaults to public.



  • static method as the calculation only depends on the two method arguments, and no other class state.



  • The use of the two method arguments makes it clear what the inputs are, rather than letting callers of the method guess how they need to pass it in.



  • Again, the use of the return type also makes it clear where the output is going - back to the caller of this method.



If you still prefer to have these four methods in a standalone class, give your Multiplication class a quick rename (CalculatorUtils?) and you can safely - and should - remove the extends Main declaration: it is simply not needed.

Second, your Calculations class looks... messy. It's messy in the sense that:

  • It's magically responsible for instantiating your Multiplication class,



  • It has poorly-named method names, and



  • It doesn't require anything from the Main class, other than the static methods.



So, once again, you will not need extends Main here. Furthermore, when you have the static methods suggested above from the now-renamed CalculatorUtils class, your switch statement should resemble something more like this:

public static float compute(String operator) {
    // assume that you have a method `getFloatInput()` that returns a float
    switch (operator) {
        case "+":
            return CalculatorUtils.plus(getFloatInput(), getFloatInput());
        case "-":
            return CalculatorUtils.minus(getFloatInput(), getFloatInput());
        case "*":
            return CalculatorUtils.multiply(getFloatInput(), getFloatInput());
        case "/":
            return CalculatorUtils.divide(getFloatInput(), getFloatInput());
        default:
            throw new IllegalArgumentException("Invalid operator.");
    }
}


In fact, you may also want to consider combining both classes together, and exposing the float inputs in the method signature too:

// final is used here to prevent sub-classing, since it's not really required
public final class CalculatorUtils {

    private CalculatorUtils() {
        // private constructor to prevent instantiation, this class
        // is meant to be used solely through its static methods
    }

    // now that we have the compute() method, perhaps we no longer
    // need these methods to be accessible outside of this class
    private static float plus(float a, float b) {
        return a + b;
    }

    // ...

    public static float compute(String operator, float a, float b) {
        switch (operator) {
            case "+":
                return plus(a, b);
            case "-":
                return minus(a, b);
            case "*":
                return multiply(a, b);
            case "/":
                return divide(a, b);
            default:
                throw new IllegalArgumentException("Invalid operator.");
        }
    }
}


Class state vs static fields

The one thing that strikes out as odd to me in your code is the heavy reliance on static fields and methods with void return types. This indicates that while you seem to think you need inheritance here (when you don't, see above section), you have forgotten about how to make effective use of class methods to pass information around explicitly, instead choosing to rely on a global state.

From a beginner's point-of-view, I t

Code Snippets

public static float plus(float a, float b) {
    return a + b;
}
public static float compute(String operator) {
    // assume that you have a method `getFloatInput()` that returns a float
    switch (operator) {
        case "+":
            return CalculatorUtils.plus(getFloatInput(), getFloatInput());
        case "-":
            return CalculatorUtils.minus(getFloatInput(), getFloatInput());
        case "*":
            return CalculatorUtils.multiply(getFloatInput(), getFloatInput());
        case "/":
            return CalculatorUtils.divide(getFloatInput(), getFloatInput());
        default:
            throw new IllegalArgumentException("Invalid operator.");
    }
}
// final is used here to prevent sub-classing, since it's not really required
public final class CalculatorUtils {

    private CalculatorUtils() {
        // private constructor to prevent instantiation, this class
        // is meant to be used solely through its static methods
    }

    // now that we have the compute() method, perhaps we no longer
    // need these methods to be accessible outside of this class
    private static float plus(float a, float b) {
        return a + b;
    }

    // ...

    public static float compute(String operator, float a, float b) {
        switch (operator) {
            case "+":
                return plus(a, b);
            case "-":
                return minus(a, b);
            case "*":
                return multiply(a, b);
            case "/":
                return divide(a, b);
            default:
                throw new IllegalArgumentException("Invalid operator.");
        }
    }
}
public void  basicAdd(){
    Result = Result + secondInput;
}
public static float plus(float a, float b) {
    return a + b;
}

Context

StackExchange Code Review Q#104461, answer score: 5

Revisions (0)

No revisions yet.