views:

86

answers:

3

So I have an abstract data type called RegionModel with a series of values (Region), each mapped to an index. It's possible to remove a number of regions by calling:

regionModel.removeRegions(index, numberOfRegionsToRemove);

My question is what's the best way to handle a call when the index is valid :

(between 0 (inclusive) and the number of Regions in the model (exclusive))

but the numberOfRegionsToRemove is invalid:

(index + regionsToRemove > the number of regions in the model)

Is it best to throw an exception like IllegalArgumentException or just to remove as many Regions as I can (all the regions from index to the end of the model)?

Sub-question: if I throw an exception what's the recommended way to unit test that the call threw the exception and left the model untouched (I'm using Java and JUnit here but I guess this isn't a Java specific question).

+1  A: 

I would say that an argument such as illegalArgumentException would be the best way to go here. If the calling code was not passing a workable value, you wouldn't necessarily want to trust that they really wanted to remove what it had them do.

Mitchel Sellers
I agree, but on the other hand I'm putting the burden of on the calling code to always check the parameters to avoid an exception being thrown. Kind of like returning null instead of an empty List (but not as bad).
Tom Martin
+2  A: 

Typically, for structures like this, you have a remove method which takes an index and if that index is outside the bounds of the items in the structure, an exception is thrown.

That being said, you should be consistent with whatever that remove method that takes a single index does. If it simply ignores incorrect indexes, then ignore it if your range exceeds (or even starts before) the indexes of the items in your structure.

casperOne
I was ignoring it for a single index but I think ignoring it would be a mistake in the multiple region case. If I change the code to throw the exception (which I'm leaning towards) it will change both methods as the code is shared. removeRegion(index) just calls removeRegions(index, 1).
Tom Martin
@Tom Martin: Which is a good idea. The key here is to be consistent across your design, and within the larger ecosystem which you are working in.
casperOne
If only the whole ecosystem was consistent! ;)
Tom Martin
@Tom Martin: If it isn't, then the best you can do is be consistent in your offering. =)
casperOne
+1  A: 

I agree with Mitchel and casperOne -- an Exception makes the most sense.

As far as unit testing is concerned, JUnit4 allows you to exceptions directly: http://www.ibm.com/developerworks/java/library/j-junit4.html

You would need only to pass parameters which are guaranteed to cause the exception, and add the correct annotation (@Test(expected=IllegalArgumentException.class)) to the JUnit test method.

Edit: As Tom Martin mentioned, JUnit 4 is a decent-sized step away from JUnit 3. It is, however, possible to also test exceptions using JUnit 3. It's just not as easy.

One of the ways I've tested exceptions is by using a try/catch block within the class itself, and embedding Assert statements within it.

Here's a simple example -- it's not complete (e.g. regionModel is assumed to be instantiated), but it should get the idea across:

public void testRemoveRegionsInvalidInputs() {
  int originalSize = regionModel.length();
  int index = 0;
  int numberOfRegionsToRemove = 1,000; // > than regionModel's current size
  try {
    regionModel.removeRegions(index, numberOfRegionsToRemove);

    // Since the exception will immediately go into the 'catch' block this code will only run if the IllegalArgumentException wasn't thrown
    Assert.assertTrue("Exception not Thrown!", false);
  }
  catch (IllegalArgumentException e) {
     Assert.assertTrue("Exception thrown, but regionModel was modified", regionModel.length() == originalSize);
  }
  catch (Exception e) {
      Assert.assertTrue("Incorrect exception thrown", false);
  }
}
bedwyr
I accepted your answer because the consensus is pointing to the exception and you mentioned the JUnit test. We probably won't be moving to JUnit 4 just yet though. As the article says "JUnit 4 is a radical new framework, not a an upgraded version of the old framework"
Tom Martin
@Tom, I included a small snippet which might help with testing exceptions in JUnit 3. I've done something similar to this in my testing, and it's worked pretty well.
bedwyr
Thanks. The test I went with declares a null exception variable and assigns it in the catch block. I then have an assertNotNull after the catch.
Tom Martin