Simple function in Java

Question

This is a JUnit asserts for this function:

    assertEquals(0, obj.generateListSize(0));
    assertEquals(1, obj.generateListSize(1));
    assertEquals(1, obj.generateListSize(2));
    assertEquals(3, obj.generateListSize(3));
    assertEquals(3, obj.generateListSize(4));
    assertEquals(5, obj.generateListSize(5));
    assertEquals(5, obj.generateListSize(6));
    assertEquals(7, obj.generateListSize(7));
    assertEquals(7, obj.generateListSize(8));
    assertEquals(7, obj.generateListSize(9));
    assertEquals(10, obj.generateListSize(10));
    assertEquals(10, obj.generateListSize(11));
    assertEquals(10, obj.generateListSize(12));

and this is my implementation:

public int generateListSize(int listSize) {
        int result;
        if (listSize < 1) {
            result = 0;
        } else if (7 < listSize && listSize < 10) {
            result = 7;
        } else if (10 <= listSize) {
            result = 10;
        } else if (listSize % 2 == 0) {
            result = listSize - 1;
        } else {
            result = listSize;
        }
        return result;
    }

This function is use to truncate list to 1,3,5,7,10 elements. When list has 8 elements then return is 7.

Answer 1

I wouldn't search for a "rule", just store the "truncate points"

private final static int[] truncateSizes = {10,7,5,3,1,0};

public int generateListSize(int listSize) {
   for(int truncateSize  : truncateSizes) {
      if (listSize >= truncateSize) { 
        return truncateSize; 
      }
   }
   throw new IllegalArgumentException("Negative list size");
}

Answer 2

This looks like a very arbitrary function to me. I'm wary of long lists of if-statements, but if the function is indeed arbitrary, I don't know how to improve that.

FWIW I'd move the else if (10 < listSize) up, to make it the second conditional. That way the boundary cases are covered first. Your unit tests still pass then.

EDIT:
I was able to create a function rule for the range [1,8]. However, this does not change the number of conditionals, as the value returned for listSize==9 remains a special case. Also, it relies on shifting bits, which I do not consider good programming practice when using function rules. For those interested, though, I include it here:

int result = 0;
if (listSize < 1) {
    result = 0;
} else if ( listSize < 9 ) {
    result = ( ( ( 1 + listSize ) >> 1 ) << 1 ) - 1;
} else if ( listSize == 9 ) {
    result = 7;
} else if (10 <= listSize) {
    result = 10;
}

In this case I put the if (10 <= listSize) at the bottom. This way the different cases are sorted by the value of listSize that they accept.

Answer 3

Since the rule has lot of exceptions, I fear it is hard to avoid all the ifs...
Here is my best attempt, not really better than your (perhaps even slightly more cryptic because of the usage of modulo...):

public int generateListSize(int listSize) {
   int result;
   if (listSize < 1) {
     result = 0;
   } else if (listSize >= 10) {
      result = 10;
   } else if (listSize == 9) {
     result = 7;
   } else {
      result = listSize - (1 - listSize % 2);
   }
   return result;
}

Answer 4

I would write the test method as:

// list sizes  = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12 };
int[] expected = { 0, 1, 1, 3, 3, 5, 5, 7, 7, 7, 10, 10, 10 };

for (int i = 0; i < results.length; ++i) {
    assertEquals(expected[i], obj.generateListSize(i));
}

Answer 5

Too many assert in one test is a bad smell. It's Assertion Roulette and you lost Defect Localization. If the first assertEquals throws an exception you don't know anything about the results of the other assert calls which could be important because they could help debugging and defect localization.

The solution is easy, use a parameterized test:

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

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

@RunWith(Parameterized.class)
public class ClientTest {

    private final int expected;
    private final int input;

    public ClientTest(final int expected, final int input) {
        this.expected = expected;
        this.input = input;

    }

    @Parameters
    public static Collection<Object[]> data() {
        final Object[][] data = new Object[][] { 
                { 0, 0 }, { 1, 1 }, { 1, 2 }, { 3, 3 }, { 3, 4 }, { 5, 5 }, { 5, 6 },
                { 7, 7 }, { 7, 8 }, { 7, 9 }, { 10, 10 }, { 10, 11 }, { 10, 12 } };
        return Arrays.asList(data);
    }

    @Test
    public void testGenerateListSize() throws Exception {
        final Client obj = new Client();
        assertEquals(expected, obj.generateListSize(input));
    }

}