3
\$\begingroup\$

Using Java 1.8, I created the following implementation to concat two int arrays:

public static int[] concat(int[] array1, int[] array2) {
    return IntStream.concat(Arrays.stream(array1), Arrays.stream(array2)).toArray();
}

The unit test I created to test this looks like this:

int[] numbers1 = new int[] {1, 4, 3, 10, 2, 11, 15, 8};
int[] numbers2 = new int[] {5, 6, 7, 9, 12, 13, 14};

@Test
public void concat() {
    int[] concatNumbers = ArrayUtils.concat(numbers1, numbers2);

    for (int i = 0; i < numbers1.length; i++) {
        assert(concatNumbers[i] == numbers1[i]);
    }

    for (int i = numbers2.length + 1; i < numbers2.length; i++) {
        assert(concatNumbers[i] - 1 == numbers1[i]);
    }
}

Questions:

  1. Although my unit test works correctly, is there a better way to unit test this method (e.g. in terms of style, cleaner code, etc.)?
  2. How could I change the actual implementation if I were to use varargs?

    public static int[] concat(int[] ... args) 
    
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for editing Jamal... How did you add the syntax coloring for concat in the first sentence? \$\endgroup\$ Jul 20, 2016 at 5:56
  • \$\begingroup\$ If you click the edit button, on any post you can see how they are formatted (and then discard the edit when you are done). Inline code formatting is done using the backtick (it's in the top left of my keyboard, but yours may vary). \$\endgroup\$
    – forsvarir
    Jul 20, 2016 at 6:06

4 Answers 4

4
\$\begingroup\$

Testing

  • concat isn't a great name for a test, it doesn't tell me anything about what it is that the test does, it simply copies the name of the method being called. Something like concatShouldReturnConcatenationForTwoPopulatedArrays might be a better description
  • Personally, for a test this simple, I'd tend to define the expected results for the test within it rather than calculating it on the fly. This makes it less likely that you'll get a failure because your test is wrong. It also allows you to use the junit array comparison.

    Assert.assertArrayEquals( expectedResult, result );
    
  • You've only got one test, I'd tend to add at least 3 more for:

    concat([], numbers2)
    concat(numbers1, [])
    concat([], [])
    
\$\endgroup\$
2
\$\begingroup\$

You should add more test cases and use more descriptive names to define what is the purpose of each test.

You should be careful with your tests, if you have errors in your tests you could get unexpected results. In your code, you are not testing your input as much as you expect. There is a bug in the test at your second loop:

// Initial value of i is greater than the breaking condition
for (int i = numbers2.length + 1; i < numbers2.length; i++) {
    // Dead code, you are not testing this part of the input
    assert(concatNumbers[i] - 1 == numbers1[i]);
}

You second loop could be:

for (int i = 0; i < numbers2.length; i++) {
    assert(concatNumbers[numbers1.length + i] == numbers2[i]);
}
\$\endgroup\$
0
\$\begingroup\$

As a first part of the test I would recommend to verify
concatNumbers.length == numbers1.length + numbers2.length

And the second for seems a total mess to me.
Did you run your test at least once?

\$\endgroup\$
0
\$\begingroup\$

The Arrays class can make things more elegant.

public void concatNumbers() {

    // Make the exptected result:
    // - Here by a reference implementation, but a manual { ... } would do too.
    int[] expectedNumbers = Arrays.copyOf(numbers1, numbers1.length + numbers2.length);
    System.arraycopy(numbers2, 0, expectedNumbers, numbers1.length, numbers2.length);

    int[] concatNumbers = ArrayUtils.concat(numbers1, numbers2);

    assertTrue(Arrays.equals(concatNumbers, expectedNumbers));
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.