Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

Your applyOperator consists solely of a switch statement. Rather than having them all assign to firstNum, then returning the value, you might as well directly return the result...

private static int applyOperator(char operand, int firstNum, int secondNum){
 switch (operand) {
 case '*':
 return firstNum * secondNum;
 case '/':
 return firstNum / secondNum;
 case '+':
 return secondNum;
 case '-':
 return -secondNum;
 }
 return firstNum;
}

This flags up some non-intuitive behaviour, both +- operators ignore the first number and only operate on the second number. This is because half of the behaviour sits in the calling method. The default behaviour for an unknown operator is also to simply return the first number. This might be acceptable for your because of the challenge parameters, however consider throwing an exception on an unknown operator...

#Bug?

Bug?

The following test fails:

assertEquals(6, ArithmethicalStatments.calc("-3*-2"));

Maybe the question sets restrictions on the values that can be used, however, assuming you're supposed to be able to use negative numbers, the way you're handling the '-' operator breaks this (the result for the above comes back as '-2').

The calculator basically consists of number tokens, separated by operators. So, you can get a number (which consists of any number of '-', followed by numerals), then a single character operator, then back to a number etc.

Each token could be separated by white space, again this doesn't work currently, adding spaces means that "5 + 3" resolves to 5, rather than 8....combined with the lack of errors reported this is confusing.

Your applyOperator consists solely of a switch statement. Rather than having them all assign to firstNum, then returning the value, you might as well directly return the result...

private static int applyOperator(char operand, int firstNum, int secondNum){
 switch (operand) {
 case '*':
 return firstNum * secondNum;
 case '/':
 return firstNum / secondNum;
 case '+':
 return secondNum;
 case '-':
 return -secondNum;
 }
 return firstNum;
}

This flags up some non-intuitive behaviour, both +- operators ignore the first number and only operate on the second number. This is because half of the behaviour sits in the calling method. The default behaviour for an unknown operator is also to simply return the first number. This might be acceptable for your because of the challenge parameters, however consider throwing an exception on an unknown operator...

#Bug?

The following test fails:

assertEquals(6, ArithmethicalStatments.calc("-3*-2"));

Maybe the question sets restrictions on the values that can be used, however, assuming you're supposed to be able to use negative numbers, the way you're handling the '-' operator breaks this (the result for the above comes back as '-2').

The calculator basically consists of number tokens, separated by operators. So, you can get a number (which consists of any number of '-', followed by numerals), then a single character operator, then back to a number etc.

Each token could be separated by white space, again this doesn't work currently, adding spaces means that "5 + 3" resolves to 5, rather than 8....combined with the lack of errors reported this is confusing.

Your applyOperator consists solely of a switch statement. Rather than having them all assign to firstNum, then returning the value, you might as well directly return the result...

private static int applyOperator(char operand, int firstNum, int secondNum){
 switch (operand) {
 case '*':
 return firstNum * secondNum;
 case '/':
 return firstNum / secondNum;
 case '+':
 return secondNum;
 case '-':
 return -secondNum;
 }
 return firstNum;
}

This flags up some non-intuitive behaviour, both +- operators ignore the first number and only operate on the second number. This is because half of the behaviour sits in the calling method. The default behaviour for an unknown operator is also to simply return the first number. This might be acceptable for your because of the challenge parameters, however consider throwing an exception on an unknown operator...

Bug?

The following test fails:

assertEquals(6, ArithmethicalStatments.calc("-3*-2"));

Maybe the question sets restrictions on the values that can be used, however, assuming you're supposed to be able to use negative numbers, the way you're handling the '-' operator breaks this (the result for the above comes back as '-2').

The calculator basically consists of number tokens, separated by operators. So, you can get a number (which consists of any number of '-', followed by numerals), then a single character operator, then back to a number etc.

Each token could be separated by white space, again this doesn't work currently, adding spaces means that "5 + 3" resolves to 5, rather than 8....combined with the lack of errors reported this is confusing.

Source Link
forsvarir
  • 11.8k
  • 7
  • 39
  • 72

Your applyOperator consists solely of a switch statement. Rather than having them all assign to firstNum, then returning the value, you might as well directly return the result...

private static int applyOperator(char operand, int firstNum, int secondNum){
 switch (operand) {
 case '*':
 return firstNum * secondNum;
 case '/':
 return firstNum / secondNum;
 case '+':
 return secondNum;
 case '-':
 return -secondNum;
 }
 return firstNum;
}

This flags up some non-intuitive behaviour, both +- operators ignore the first number and only operate on the second number. This is because half of the behaviour sits in the calling method. The default behaviour for an unknown operator is also to simply return the first number. This might be acceptable for your because of the challenge parameters, however consider throwing an exception on an unknown operator...

#Bug?

The following test fails:

assertEquals(6, ArithmethicalStatments.calc("-3*-2"));

Maybe the question sets restrictions on the values that can be used, however, assuming you're supposed to be able to use negative numbers, the way you're handling the '-' operator breaks this (the result for the above comes back as '-2').

The calculator basically consists of number tokens, separated by operators. So, you can get a number (which consists of any number of '-', followed by numerals), then a single character operator, then back to a number etc.

Each token could be separated by white space, again this doesn't work currently, adding spaces means that "5 + 3" resolves to 5, rather than 8....combined with the lack of errors reported this is confusing.

lang-java

AltStyle によって変換されたページ (->オリジナル) /