current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite

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)
share|improve this question
    
Thanks for editing Jamal... How did you add the syntax coloring for concat in the first sentence? – socal_javaguy Jul 20 at 5:56
    
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). – forsvarir Jul 20 at 6:06
up vote 4 down vote

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([], [])
share|improve this answer
up vote 2 down vote

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]);
}
share|improve this answer
up vote 0 down vote

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?

share|improve this answer
up vote 0 down vote

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));
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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