Simple calculator to evaluate arithmetic expressions

Question

I've studied Java for 2 and a half months. Got bored and decided to make a basic calculator that takes any basic expression with 2 integers/doubles. E.g. 5.5x10, 5+5, 5-2.5, 6/2.

Is this a good way to approach this task? How could it be improved?

The subtraction has a "+" because substring takes into account the "-" as part of the second substring and the parseDouble turns that into a minus therefor two minuses would make a plus. I corrected the multiplication and division and i left the addition and subtraction how it was. I would correct all in the future.

/*
 * Author v0lk
 * Simple calculator program
 * 10/01/18
 */
import java.util.*;
public class Calc {
 public static void main(String[] args) {
 Calc c = new Calc();
 c.repeatCalc();
 }
 Scanner input = new Scanner(System.in);
 public void repeatCalc() {
 while (1>0) {
 calculator();
 }
 }
 public void calculator() {
 double result=0;
 String problm = "";
 System.out.println("Use: \n\"/\" to divide \n\"x\" to multiply \n\"+\" for addition \n\"-\" to substract \n\n>");
 problm = input.nextLine();
 for (int i=0; i<problm.length();i++) {
 if (problm.charAt(i)=='/') {
 result = Double.parseDouble(problm.substring(0, i)) / Double.parseDouble(problm.substring(i+1,problm.length()));
 }
 if (problm.charAt(i)=='-') {
 result = Double.parseDouble(problm.substring(0, i)) + Double.parseDouble(problm.substring(i,problm.length()));
 }
 else if (problm.charAt(i)=='x') {
 result = Double.parseDouble(problm.substring(0, i)) * Double.parseDouble(problm.substring(i+1,problm.length()));
 }
 else if (problm.charAt(i)=='+') {
 result = Double.parseDouble(problm.substring(0, i)) + Double.parseDouble(problm.substring(i,problm.length()));
 }
 }
 System.out.println("Answer is: " + result);
 }
}

• Timothy Truckle: while being new to Java you might spend some time with the official Java Tutorial: docs.oracle.com/javase/tutorial

Answer 1

Thanks for sharing your code!

Pros

• You actually created an instance of your class instead of using static methods
• you have more than one method.

Cons

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

Naming Conventions

Please read (and follow) the Java Naming Conventions

E.g.: method names should be or start with a verb. Your method calculator() should better be named calculate().

Care for correct spelling

You named a variable problm. What is that? I suspect you misspelled problem, but it could something completely different.

Declare variables as late as possible

Your method calculator() has two variables immediately declared at the beginning of the method. you first variable is result but it is the last one referenced. It is not needed untill the start of the for loop. Therefore it should be declared there.

The reason is that "late declaration" simplifies the use of your IDEs automated refactorings. One of them is extract method. You can use it to put the for loop into a separate method. If you do it on your code as it is you would get this:

public void calculator() {
 double result=0;
 String problm = "";
 System.out.println("Use: \n\"/\" to divide \n\"x\" to multiply \n\"+\" for addition \n\"-\" to substract \n\n>");
 problm = input.nextLine();
 result = calculateResult(problm, result);
 System.out.println("Answer is: " + result);
}
private double calculateResult(String problm, double result){
 for (int i=0; i<problm.length();i++) { 
 // calculate the actual result...
 }
 return result;
}

You new method got an useless parameter.

If you had declared the variable result before the loop you could hav included it in the extracted patr and your code would be like this:

public void calculator() {
 System.out.println("Use: \n\"/\" to divide \n\"x\" to multiply \n\"+\" for addition \n\"-\" to substract \n\n>");
 String problm = input.nextLine();
 double result = calculateResult(problm);
 System.out.println("Answer is: " + result);
}
private double calculateResult(String problm){
 double result=0;
 for (int i=0; i<problm.length();i++) { 
 // calculate the actual result...
 }
 return result;
}

• v0lk: Thanks for the feedback, took some really good notes from that! The reason why It remained called "problm" was because I changed the way I approached it. Originally I had 2 similar variables and "problem" was already used. I'll remember to edit it in the future

Answer 2

Single Responsibility

Calc does everything which breaks the single responsibility principle. By everything, I mean:

• gathers user input
• parses the user input
• calculates the result
• displays the result

I suggest breaking this into at least two classes:

• ConsoleCalculator - has the main method, and interacts with the user to gather console input and display the result to the console.
• Calculator - parses problem and calculates the result.

With the appropriate separation, Calculator is now re-usable. For now, the user interacts with it through the console. It could be used by some other UI approach, such as SWING or web service.

Arguably, it might be useful to have the parsing of problem in a separate class, with Calculator acting on the results of the parser. For example, suppose problem was "1+2*3". What would need to be changed to handle that?

Single responsibility can be applied to a method as well. calculator() could be delegate to methods such as displayMenu(), getProblemFromUser(), etc. (Some of those methods could delegate to a different object.)

Program Control

The program runs forever - consider adding a menu option to gracefully exit.

When the program ends input, a Scanner, should be closed.

There is a missing else in the else-if construct:

if (problm.charAt(i)=='/') {
 ...
}
if (problm.charAt(i)=='-') { // else missing here
 ...
}
else if (problm.charAt(i)=='x') {
 ...
}
else if (problm.charAt(i)=='+') {
 ...
}

Readability

There is somewhat nit picky, but there are extra blank lanes. For example this code block:

 for (int i=0; i<problm.length();i++) {
 if (problm.charAt(i)=='/') {
 result = Double.parseDouble(problm.substring(0, i)) / Double.parseDouble(problm.substring(i+1,problm.length()));
 }
 if (problm.charAt(i)=='-') {
 result = Double.parseDouble(problm.substring(0, i)) + Double.parseDouble(problm.substring(i,problm.length()));
 }
 else if (problm.charAt(i)=='x') {
 result = Double.parseDouble(problm.substring(0, i)) * Double.parseDouble(problm.substring(i+1,problm.length()));
 }
 else if (problm.charAt(i)=='+') {
 result = Double.parseDouble(problm.substring(0, i)) + Double.parseDouble(problm.substring(i,problm.length()));
 }
 }

is more readable as:

 for (int i=0; i<problm.length();i++) {
 if (problm.charAt(i)=='/') {
 result = Double.parseDouble(problm.substring(0, i)) / Double.parseDouble(problm.substring(i+1,problm.length()));
 }
 else if (problm.charAt(i)=='-') {
 result = Double.parseDouble(problm.substring(0, i)) + Double.parseDouble(problm.substring(i,problm.length()));
 }
 else if (problm.charAt(i)=='x') {
 result = Double.parseDouble(problm.substring(0, i)) * Double.parseDouble(problm.substring(i+1,problm.length()));
 }
 else if (problm.charAt(i)=='+') {
 result = Double.parseDouble(problm.substring(0, i)) + Double.parseDouble(problm.substring(i,problm.length()));
 }
 }

• v0lk: Thanks for your reply! I will take in to account the things you said for when I add more features