patternjavaModerate
Simple calculator that seems inefficient
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
```
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:
-
Naming is simply awful.
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:
-
You have several methods to extract out of
-
Exception handling is pretty tangled, hard to follow. You have a
I'll leave the rest to other reviewers more familiar with java :)
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.