1

If I make a class, that in the constructor accepts an array or collection, but this array or collection is optional (the parameter can be null), and then I store that collection in the object in question, in the constructor i need something like:

this.coll = coll == null ? null : coll.clone();

Should I unit-test such kind of code? like check if it works when passing null or not?

4

Technically, what you should test depends on your test goals. But in general, you should try to test everything that can go wrong. Here:

this.coll = coll == null ? null : coll.clone();
  • You might have forgotten to use the coll parameter, i.e. the above statement is missing. This can be checked by a test that depends on the coll provided to the constructor.

  • You might have forgotten to clone the parameter:

    this.coll = coll;
    

    This can be checked by a test that modifies the parameter coll and compares the result with the object-owned coll. They should be different after the modification.

  • You might have forgotten to handle the null case:

    this.coll = coll.clone();
    

    This can be checked by a test that omits this parameter/provides a null value. Usually we'd expect an exception, here a null is OK.

  • Another thing that could go wrong is that you create a null object or default instance when a null is encountered, e.g.:

    this.coll = coll == null ? new Collection() : coll.clone();
    

    If your code is correct, you want to ensure that an actual null here, not a default instance.

That's already four test cases that would be sensible for this simple line of code. Using a code coverage tool can help to detect uncovered cases in your code, in particular if you also look at branch coverage. Some tools have problems with expression-level control-flow (?:, &&, ||) so it's better (and more readable for humans, too!) to use statement-level conditionals:

if (coll != null) {
    coll = coll.clone();
}
this.coll = coll;
  • Hmmf, well buggy tools aside, this human has no issue looking at 'expression-level control-flow' when it's well written like this. The if however needs to make up it's mind regarding use of this for the assignments. – candied_orange Jan 28 '17 at 16:49
  • @CandiedOrange I once had a code golfing phase but that has passed. Now I try to put as little information on each line as possible. I like ternaries for data flow, but control flow (as in this case) is better expressed on a statement level. The above conditional has made up it's mind, with coll referring to the parameter, not the instance member which is assigned on the next line. – amon Jan 28 '17 at 16:59
  • I do not test for the case of missing assignment, because I would have to test for all... seven or so parameters this way, but I currently test every other of those cases mentioned in the answer. – Michał Zegan Jan 28 '17 at 17:57
  • @MichałZegan That's OK, since the test for assignment is implicit to all the other tests. I tend to include it for clarity, but it's superfluous, strictly speaking. Testing for null and for cloning is the only important part here. – amon Jan 28 '17 at 19:08
  • but most parameters of this constructor are neither collections not arrays, and I also do not test for assignment for them :) – Michał Zegan Jan 28 '17 at 19:13
1

If it should throw an exception when you pass the constructor a null it's good to have a test that only turns green when passing null produces an exception.

Pretty much any behavior you expect out of a class is a candidate for unit testing. This is true for constructors as well.

  • you know, it is a case where null and not null is ok, and the only reason I need to test anything is this one, that you cannot just this.col=col, as this will share the underlying array or collection, and is dangerous, so it needs a conditional clone. – Michał Zegan Jan 28 '17 at 17:56

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.