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

Simple calculator that seems inefficient

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

Problem

I have a simple calculator program that needs some reviewing. Is this as efficient as it can get, or is there a better way?

```
public static void main(String[] args) {
Scanner sc = new Scanner(System.in);
System.out.println("Welcome to the Calculator!");
do {
System.out.println("\nType a calculation:");
String s = sc.nextLine();
try {
System.out.println("\nThe answer is: " + calculate(s));
} catch(Exception e) {
System.out.println("Oops! Illegal calculation.");
}
} while(doAgain(sc));
System.out.println("\nThank you for using the calculator!");
sc.close();
}

private static boolean doAgain(Scanner sc) {
while(true) {
System.out.println("\nAgain?");
String s = sc.nextLine();
if(s.equalsIgnoreCase("y") || s.equalsIgnoreCase("yes") || s.equalsIgnoreCase("t") || s.equalsIgnoreCase("true"))
return true;
else if(s.equalsIgnoreCase("n") || s.equalsIgnoreCase("n") || s.equalsIgnoreCase("f") || s.equalsIgnoreCase("false"))
return false;
}
}

public static Double calculate(String s) throws IllegalArgumentException {
IllegalArgumentException exception = new IllegalArgumentException("Illegal Calculation.");
String a = "";
ArrayList o = new ArrayList(s.length());
ArrayList z = new ArrayList(s.length());
double j = 0.0;
double k = 0.0;
int iInARow = 0;
int sInARow = 0;
boolean hasDecimal = false;
boolean out = false;
for(int i = 0 ; i = 2) {
Double dou = k;
Character character = dou.toString().charAt(dou.toString().lastIndexOf(".") + 1);
if(!character.equals('0'))
j = Double.parseDouble(k + d + (int) j);
else
j = Double.parseDouble((int) k + d + (int) j);
}
d = "";
try {
Integer.parseInt(s.charAt(i + 1) + "");
} catch(Ex

Solution

It started off pretty well: main and doAgain are very reasonably sized.

calculate is a monster though. It's so crippled I don't even want to imagine what maintaining that code would be. Reviewing reading it is hard, for several reasons:

-
Naming is simply awful.

String a = "";
ArrayList o = new ArrayList(s.length());
ArrayList z = new ArrayList(s.length());
double j = 0.0;
double k = 0.0;
int iInARow = 0;
int sInARow = 0;


The human brain my poor little fried brain isn't very good at keeping track of the identifier-meaning mappings many single-letter variables. It's much easier when the identifiers themselves are descriprive enough to convey their meaning.

I see only one such variable:

boolean hasDecimal = false;


-
You have several methods to extract out of calculate - one for each operation you want to support. Don't cram everything into a calculate function.

-
Exception handling is pretty tangled, hard to follow. You have a try/catch block inside a catch block, that seems to be chained (hard to tell because of indentation).

I'll leave the rest to other reviewers more familiar with java :)

Code Snippets

String a = "";
ArrayList<Double> o = new ArrayList<Double>(s.length());
ArrayList<String> z = new ArrayList<String>(s.length());
double j = 0.0;
double k = 0.0;
int iInARow = 0;
int sInARow = 0;
boolean hasDecimal = false;

Context

StackExchange Code Review Q#57134, answer score: 10

Revisions (0)

No revisions yet.