Raindrops in Java

Question

Problem Statement:

Write a program that converts a number to a string, the contents of which depends on the number's prime factors.

• If the number contains 3 as a prime factor, output 'Pling'.
• If the number contains 5 as a prime factor, output 'Plang'.
• If the number contains 7 as a prime factor, output 'Plong'.
• If the number does not contain 3, 5, or 7 as a prime factor, just pass the number's digits straight through.

Code:

public class Raindrops {

  private Raindrops() {}

  public static String convert(int number) {
    // Pre-condition.
    if (number < 0) {
      throw new IllegalArgumentException("Input cannot be negative.");
    }
    String result = "";
    for (Raindrop drop : Raindrop.values()) {
      if (drop.hasPrimeFactor(number)) {
        result += drop.toString();
      }
    }
    //if (result % 3 == 0)
      //result += "Pling";
    //if (number % 5 == 0)
      //result += "Plang";
    //if (number % 7 == 0)
      //result += "Plong";
    if (result.isEmpty()) {
      result = "" + number;
    }
    checkPostCondition(result, number);
    return result;
  }

  private static void checkPostCondition(String result, int number) {
    assert(result.contains("Pling") ||
        result.contains("Plang") ||
        result.contains("Plong") ||
        result.contains("" + number));
  }

  private enum Raindrop {
    Pling(3),
    Plang(5),
    Plong(7);

    private final int primeFactor;

    private Raindrop(int primeFactor) {
      this.primeFactor = primeFactor;
    }

    public boolean hasPrimeFactor(int number) {
      return number % primeFactor == 0;
    }
  }
}

Test Suite:

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

import java.util.Arrays;
import java.util.Collection;

import static org.junit.Assert.assertEquals;

@RunWith(Parameterized.class)
public class RaindropsTest {

    private int input;
    private String expectedOutput;

    @Parameters
    public static Collection<Object[]> data() {
        return Arrays.asList(new Object[][]{
                // Non-primes
                {1, "1"},
                {52, "52"},
                {12121, "12121"},

                // Numbers with 3 as a prime factor
                {3, "Pling"},
                {6, "Pling"},
                {9, "Pling"},

                // Numbers with 5 as a prime factor
                {5, "Plang"},
                {10, "Plang"},
                {25, "Plang"},

                // Numbers with 7 as a prime factor
                {7, "Plong"},
                {14, "Plong"},
                {49, "Plong"},

                // Numbers with multiple activating prime factors
                {15, "PlingPlang"},
                {21, "PlingPlong"},
                {35, "PlangPlong"},
                {105, "PlingPlangPlong"},
        });
    }

    public RaindropsTest(int input, String expectedOutput) {
        this.input = input;
        this.expectedOutput = expectedOutput;
    }

    @Test
    public void test() {
        assertEquals(expectedOutput, Raindrops.convert(input));
    }
}

Notes:

Although the solution was quite simple (see my commented code), I am trying to push myself and experiment with program correctness and flexibility, hence the final solution may seem over-engineered.

Reference

Answer 1

Post Condition

The post condition would pass even if the result is something like "PlingPlongNUMBER". (If I understood correctly, then the result should be either NUMBER or any combination of Pling/Plong/Plang, but never both.) Therefore, I suggest a post condition like the following (not tested!):

assert(((result.contains("Pling") || result.contains("Plang") || result.contains("Plong")) && !result.contains("" + number)) ||
    (result.contains("" + number) && (!result.contains("Pling") && !result.contains("Plang") && !result.contains("Plong")))));

OOP changes

Since you are exercising OOP (and don't mind some over-engineering ;)) , you could consider adding another RainDrop, for dealing with the case that no prime factor matches (e.g. RainDrop(-1)). This would require the following modifications

• Rename hasPrimeFactor to something reflect more accurately what it does (e.g. processNumber).

• For the enum -1, processNumber would check that the parameter is NOT divisible by either 3, 5 or 7, and return the number as a string if that is the case.

• Possible performance improvement: you might want to build a cache for divisibility with 3, 5 and 7, in case you are worried that modulo is calculated twice for each dividend. (E.g. cache table for 3 can be a HashMap, that for each already seen number tells if it is divisible with 3 or not. Same for 5 and 7.) Caveat: I did not verify that doing the lookup in the HashMap (let alone lookup + storage!) is faster than doing the modulo division, so you might actually lose performance in this way. The idea is rather to experiment with a way of caching, in case you were doing a really expensive operation.

As I said above, I'm not saying that this suggested change is necessarily better than the code you have now (in fact it is a bit more over-engineered). It is rather a way of exploring how to further OOP-fy your code.

Test Code

Let me first say, that I find it a really positive thing that you write your code with testing in mind, and add unit tests to cover (almost) all the cases. That said, some remarks:

• Corner cases: I suggest adding tests for 0 and -1 as well. (Maybe -1 is tricky, since you expect an exception, all the same it is worth the effort.)

• Comment about // Non-primes: while technically correct (the input numbers are non-primes), the point is that the numbers are not divisible by 3, 5 or 7, not that they are non-primes. So, I suggest updating the comment accordingly.