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 calculation methods:
public class Multiplication extends Main {
public void basicMult(){
Result = Result * secondInput;
}
public void basicSubt(){
Result = Result - secondInput;
}
public void basicAdd(){
Result = Result + secondInput;
}
public void basicDiv(){
if(secondInput == 0.0f){
System.out.println("Cannot divide by 0");
}
else{
Result = Result / secondInput;
}
}
}
Class to switch between each calculation:
public class Calculations extends Main{
Multiplication calculation = new Multiplication();
String getCalc;
public void sortCalc(){
getCalc = getInput("'+' '-' '*' or '/' or 'Done'");
calcSwitch();
}
private void calcSwitch(){
switch(getCalc){
case "+":
getIteration();
calculation.basicAdd();
sortCalc();
break;
case "-":
getIteration();
calculation.basicSubt();
sortCalc();
break;
case "*":
getIteration();
calculation.basicMult();
sortCalc();
break;
case "/":
getIteration();
calculation.basicDiv();
sortCalc();
case "Done":
break;
case "default":
System.out.println("That isn't a mathematical function");
sortCalc();
break;
}
}
}
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
publicas it is likely to be used outside the class, not because everything defaults topublic. staticmethod 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
Multiplicationclass, - It has poorly-named method names, and
- It doesn’t require anything from the
Mainclass, other than thestaticmethods.
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 think the biggest obstacle to using static fields (not methods) throughout is that it makes your own code harder to understand in the long run. The reader has to memorize where these fields appear, and how they are modified throughout the codebase. If I may draw from my earlier example again, compare your Multiplications method:
public void basicAdd(){
Result = Result + secondInput;
}
- Where are
ResultandsecondInputfrom, and their type? - How can the method caller retrieve the result of incrementing
Result?
With my suggestion:
public static float plus(float a, float b) {
return a + b;
}
- This method takes in two
floatvalues. - It returns the result of adding the values together directly to the method caller.
Doing iterations
You have a weird way of iterating through methods. Instead of using loop structures, you have your methods repeatedly calling each other at the end as such (// Here... // And here.):
public class Calculations extends Main{
public void sortCalc(){
getCalc = getInput("'+' '-' '*' or '/' or 'Done'");
calcSwitch(); // Here...
}
private void calcSwitch(){
switch(getCalc){
case "+":
getIteration();
calculation.basicAdd();
sortCalc(); // And here.
break;
// ...
}
}
The downside of methods calling each other is that it’s inefficient since the JVM will be also have to be busy with increasing the call stack. For example, this is how your oddly-named tryCatchMethod() can be improved:
// instead of wrapping a BufferedReader/InputStreamReader over System.in,
// you can also consider something like Scanner scanner = new Scanner(System.in);
// and pass that *one* instance here
private static float getFloatInput(Scanner scanner) {
while (scanner.hasNextLine()) {
try {
return Float.parseFloat(scanner.nextLine());
} catch (NumberFormatException e) {
System.err.println("Invalid float input.");
}
}
throw new RuntimeException("No input encountered.");
}
Putting it altogether
Given the relatively simple approach we have here, you probably do not require any class state, since you can easily pass your numeric inputs around well-specified methods to get your results. For example, in your main() method, you only require something like the following (try-with-resources is recommended on the Scanner instance since you appear to be at least on Java 7):
public class Main {
public static void main(String[] args) {
try (Scanner scanner = new Scanner(System.in)) {
float result = getFloatInput(scanner);
String operator;
while (!(operator = getOperatorInput(scanner)).equals("Done")) {
result = CalculatorUtils.compute(operator, result, getFloatInput(scanner));
// print intermediate result here?
}
System.out.println("The result is :" + result);
}
}
}