Java Kata - String Calculator

Question

I started learning Java recently. Currently I'm trying to improve my spaghetti code by solving different programming puzzles.

It would be amazing if someone could give me feedback on the String Calculator Kata specified at Coding Dojo.

Any feedback for improvement would be awesome...

// StringCalculator.java
public class StringCalculator {
 private float result;
 private String customDelimiter;
 private static final String DEFAULT_DELIMITER = ",";
 private static final String NEWLINE = "\n";
 private static final String CUSTOM_DELIMITER_PREFIX = "/";
 private static final String CUSTOM_DELIMITER_SUFFIX = NEWLINE;
 StringCalculator() {
 result = 0;
 customDelimiter = "";
 }
 public String sum(String numbers) {
 if (numbers.isEmpty())
 return String.format("%.0f", result);
 if (isInvalidLastCharacterIn(numbers))
 return "Number expected but EOF found.";
 if (numbers.startsWith(CUSTOM_DELIMITER_PREFIX))
 numbers = setCustomDelimiter(numbers);
 if (isNewlineAtInvalidPositionIn(numbers))
 return String.format("Number expected but '\n' found at position %d.", numbers.lastIndexOf('\n'));
 if (containsNegative(numbers).length() > 0)
 return String.format("Negative not allowed: %s", containsNegative(numbers));
 calculateSumOf(getStringArray(numbers));
 return hasDecimalPlaces() ? printFloat() : printInteger();
 }
 private boolean isInvalidLastCharacterIn(String numbers) {
 return Character.digit(numbers.charAt(numbers.length() - 1), 10) < 0;
 }
 private boolean isNewlineAtInvalidPositionIn(String numbers) {
 return numbers.lastIndexOf(NEWLINE) > numbers.lastIndexOf(DEFAULT_DELIMITER);
 }
 private StringBuilder containsNegative(String numbers) {
 StringBuilder negativeNumbers = new StringBuilder();
 for (String number : getStringArray(numbers))
 if (Float.valueOf(number) < 0) negativeNumbers.append(number + ",");
 boolean commaIsLastChar = negativeNumbers.length() > 0 && negativeNumbers.charAt(negativeNumbers.length() -1) == ',';
 return commaIsLastChar ? negativeNumbers.deleteCharAt(negativeNumbers.length() - 1)
 : negativeNumbers;
 }
 private String setCustomDelimiter(String numbers) {
 int customDelimiterStart = numbers.lastIndexOf(CUSTOM_DELIMITER_PREFIX) + 1;
 int customDelimiterEnd = numbers.indexOf(CUSTOM_DELIMITER_SUFFIX);
 customDelimiter = numbers.substring(customDelimiterStart, customDelimiterEnd);
 return numbers.substring(customDelimiterEnd + 1).replace(customDelimiter, DEFAULT_DELIMITER);
 }
 private String[] getStringArray(String numbers) {
 return numbers.replace(NEWLINE, DEFAULT_DELIMITER).split(DEFAULT_DELIMITER);
 }
 private void calculateSumOf(String[] numbers) {
 for (String number : numbers)
 result = Float.sum(result, Float.parseFloat(number));
 }
 private boolean hasDecimalPlaces() {
 return result % 1 != 0;
 }
 private String printFloat() {
 return Float.toString((float) (Math.round(result * 100.0) / 100.0));
 }
 private String printInteger() {
 return String.valueOf((int) result);
 }
}

// StringCalculatorShould.java
import org.junit.Test;
import static org.junit.Assert.assertEquals;
public class StringCalculatorShould {
 @Test
 public void
 return_0_when_input_is_empty() {
 assertEquals("0", given(""));
 }
 @Test
 public void
 return_3_when_input_is_1_2() {
 assertEquals("3", given("1,2"));
 }
 @Test
 public void
 sum_floats_and_return_float() {
 assertEquals("6.6", given("2.2,4.4"));
 }
 @Test
 public void
 treat_newLine_as_a_delimiter() {
 assertEquals("6", given("1\n2,3"));
 }
 @Test
 public void
 return_error_msg_when_newLine_at_invalid_position() {
 assertEquals("Number expected but '\n' found at position 6.", given("1,2,5,\n3"));
 }
 @Test
 public void
 return_error_msg_when_delimiter_at_last_position() {
 assertEquals("Number expected but EOF found.", given("2,3,4.2,"));
 }
 @Test
 public void
 return_correct_sum_when_custom_delimiter_is_used() {
 assertEquals("3", given("//;\n1;2"));
 assertEquals("3", given("//|\n1|2"));
 assertEquals("8", given("//@@\n1@@2@@5"));
 assertEquals("5", given("//sep\n2sep3"));
 }
 @Test
 public void
 return_string_of_negative_numbers_when_negative_numbers_are_used_as_input() {
 assertEquals("Negative not allowed: -1", given("-1,2"));
 assertEquals("Negative not allowed: -4,-5", given("2,-4,-5"));
 }
 private String given(String number) {
 StringCalculator stringCalculator = new StringCalculator();
 return stringCalculator.sum(number);
 }
}

• markspace: You asked below for good ideas for problems to work on. First, Learning Java by O'Rielly was a good resource when I first learned Java, and the examples at least were far more cogent that most other resources. Second, you might try The C++ Programming Language by Bjarne Stroustrup. It's written as a text book, doesn't suck, and translate the C++ problems he gives to Java. It will be illuminating at least. Good luck.

Answer 1

The challenge

First of all, a method called String add(String number) is just wrong in every way that you look at it. It may be a "classic" Kata, but for me that's classic stupidity, especially if you consider that the method needs to add the numbers within the number given.

---

The challenge really leads you into returning strings for anything. In program jargon, we call that stringly typed and it is something that should be avoided.

---

You've created a sum method which is great, but it is failing the challenge which requires an add method. I'd always keep to the requirements.

That also goes for "Allow the add method to handle an unknow number of arguments." (I didn't create the typo). However, I expect that the author meant that the single string can contain multiple arguments. No way to know for sure.

---

Using the same output for errors and common return values should be avoided, printing out to a different stream (standard error rather than standard out) would be one method of handling that.

---

The idea that a class would return multiple errors for the same input is weird, most classes would just generate a single error, also because one error may beget other errors: fixing the first one may fix both (e.g. you could see this for using a dot instead of a comma as separator).

---

The challenge basically also requires you to allow trash input, and then return a correct result. Generally we test our input before we accept it, explicitly not allowing trash input. Otherwise we default to "garbage in / garbage out" principles.

---

The challenge isn't symmetric in the sense that empty input should return 0 as return value, but it doesn't allow empty number strings.

---

Similarly, you need to include a position when a number is expected at a certain position, but then you also need to return "negative number" errors without a position.

Class design

Having a calculator class is alright, but it is weird that the calculator stores the result and a custom delimiter. Both values seem to be specific to the sum method, which means that they should be kept local to the sum method.

An example of a good field would be a delimiter, which could default to the DELIMITER constant.

I could see an Addition class that has an add(float) method and returns a total class, but the assignment is really more about input validation than anything else. So a total variable local to the add / sum method seems more logical.

---

The assignment clearly states (under "error management") that multiple errors may need to be returned. To me that shows that you may want to generate a list of errors, and that you should keep on testing rather than using return on any error.

---

This is about test driven design. You probably want to test the validation strategies separately. If you create public static validation methods then you can create separate tests for them (I'd also blame the setup of the challenge for this). Creating a Validator class and a separate ValidatorResult may also be a good idea if your class gets too complicated otherwise.

Code review

if (numbers.isEmpty())
 return String.format("%.0f", result);

Always try and use braces after an if branch:

if (numbers.isEmpty()) {
 return String.format("%.0f", result);
}

---

if (numbers.startsWith(CUSTOM_DELIMITER_PREFIX))
 numbers = setCustomDelimiter(numbers);

Try not to mix error handling with normal code. First check for errors, then handle the correct values. You don't want to perform any side effects (setting the custom delimiter) before you are sure that the input is correct.

---

private boolean isNewlineAtInvalidPositionIn(String numbers) {
 return numbers.lastIndexOf(NEWLINE) > numbers.lastIndexOf(DEFAULT_DELIMITER);
}

If I look at the assignment you may need to store the location of the errors as well. Returning just a boolean is probably not going to do this; you may need to perform the same test again to find the error position.

---

private StringBuilder containsNegative(String numbers) {

Now a private method has a bit more leeway when it comes to side effects and return values / types. However, a contains method should return a boolean and not a StringBuilder. A StringBuilder should really never be returned; it could be consumed as parameter though.

---

private String setCustomDelimiter(String numbers) {

Please answer these questions, without looking at your code:

• What would setCustomDelimiter require as parameter?
• What does a setter use as return value?

---

private String[] getStringArray(String numbers) {

No, the method is named badly. Call it separateInputString or something similar, but getStringArray doesn't specify what the method does, it just indicates what it returns.

---

Similarly:

private void calculateSumOf(String[] numbers) {
 for (String number : numbers)
 result = Float.sum(result, Float.parseFloat(number));
}

No return value? Why not just use the + operator?

private boolean hasDecimalPlaces() {
 return result % 1 != 0;
}

Again working on the result. But I don't get the calculation either. This is isOdd it seems to me.

---

private String printFloat() {
 return Float.toString((float) (Math.round(result * 100.0) / 100.0));
}

Generally we try and print to stream. This is about formatting a float. You could use formatResultAsFloat(). This should also give you a hint that you could use String.format instead of performing such coding yourself.

Model

It seems to me that you've started coding a little bit early. First you should try and create some model first. E.g. you could decide to first validate the input string (as you also need positions for the errors), then split, then perform the calculations on the separate values.

Conclusion

Do I think there is a lot to be improved? Certainly. Do I think that this is spaghetti code? Certainly not. Furthermore, the identifiers are well named, and if you don't include the way parameters are handled, so are the methods. The code is nicely split up, even if some reordering may be a good idea.

The Kata or challenge on the other hand is beyond saving and should be destroyed. But that's just my opinion.

• LindsayMills: Thank you so much for the effort! I'll go about the code again to fix and rethink with that input in mind
• Maarten Bodewes: I'd not overspend time on this; the use case, the error handling, the whole idea of what this does is all terrible. Don't trust that entire site. The "peoples" there seem well willing enthusiasts, but this cannot have been written by any expert.
• LindsayMills: Hope this is not off-topic, but can you recommend any resources / material that I can use instead to practice? I liked the idea of small coding exercises, but without mentors / senior devs it's not easy to progress
• Maarten Bodewes: To be honest, no, I cannot. I've been out of that game for too long. Maybe check out this site for other contests and ideas. Or try and find something that you care about and try that. In the end the practice is what makes perfect. Do however try and find problems that seem logical. Having numbers separated by a separator that can be defined in the same string is not a logical problem; it is needlessly complex and allows too many exceptional circumstances.
• Maarten Bodewes: Small note: if I look at the assignment it seems that it always needs to distinguish between different types. That means that it may be a good idea to tokenize: look at the syntax before anything else. For instance, at each position of an element, you look if it is a number or separator rather than assuming a number or separator at a specific location. Bottom up instead of top down approach, in other words. This is a separator, but I'm expecting a number rather than I'm looking for a number, but this is not a number.

Answer 2

In addition to @MaartenBodewes brilliant answer some words to your unit tests:

what I like

• the fact that you have unit tests at all.
• the naming is well thought of.

what I don't like

• Do not reduce the content of a test method to a single line.

  Unit tests are documentation. You should write your unit test in a way, that they describe the testes expectation in detail. One formal way to do that is to keep the arrange, act, assert structure of each test.

   @Test
   public void return_0_when_input_is_empty() {
   // arrange 
   String input = "";
   String expectedOutput = "0"
   // act 
   String testResult = new StringCalculator().sum(input);
   // assert 
   assertEquals(expectedOutput, testResult);
   }
  

  For the same reason prefer the assert* methos with that extra String parameter so that you get a more descriptive output when the test fails:

   @Test
   public void return_0_when_input_is_empty() {
   // ...
   // assert 
   assertEquals("empty string",expectedOutput, testResult);
   }
  

• verify only one expectation on your code with every test method.

  While the testing framework will execute all test methods regardless of whether they fail or not a test method will stop a the first failing assert instruction. This will reduce the information you get about the reason why your code failed.

   @Test
   public void
   return_correct_sum_when_delimiter_is_semicolon() {
   assertEquals("3", given("//;\n1;2"));
   }
   @Test
   public void
   return_correct_sum_when_delimiter_is_pipe() {
   assertEquals("3", given("//|\n1|2"));
   }
   @Test
   public void
   return_correct_sum_when_delimiter_is_newline() {
   assertEquals("8", given("//@@\n1@@2@@5"));
   }
   @Test
   public void
   return_correct_sum_when_delimiter_is_string() {
   assertEquals("5", given("//sep\n2sep3"));
   }
  

  For the time being this will force you to create more test methods, but soon you'll find out how to do this with parameterized tests.

  Disclaimer "one assert per test method" is a rule of thumb, not a natural law. There might be situations where more that one assert per method is needed, but if you're going to do that you should have a really good reason to do so.

• LindsayMills: Thanks a lot for the feedback! I'm gonna change my test suit accordingly