API Design - Mixing in pre-condition checks for index out of bounds?

Question

I am designing an API that does something like this:

// Drop $howMany items from after $from 
def dropElements[T](array: Array[T], from: Int, howMany: Int)

The expected behaviour is that howMany should be non-negative and if howMany is zero, it should not do any modification. I have 2 ways to implement this:

def dropElements[T](array: Array[T], from: Int, howMany: Int) {
  assert(howMany >= 0);
  if (howMany == 0) return;
  assert(0 <= from && from < array.length);
  ....   
}

OR:

def dropElements[T](array: Array[T], from: Int, howMany: Int) {
  assert(howMany >= 0);
  assert(0 <= from && from < array.length);
  if (howMany == 0) return;
  ....   
}

I am in favor of the 2nd approach (declaring your preconditions upfront) vs the 1st approach but I was pointed out that the 1st approach is more respectful of the requirements when howMany = 0.

Any thoughts or pros/cons? I am asking as a designer of a standard collections library

Answer 1

My thoughts, for what it's worth:

I think it's more consistent to do the bound checking of from in all cases. An out-of-bound from is probably a mistake in the caller code whatever the value of howMany. For me it's preferable to fail-fast in that case.

I don't really see this as a violation of the requirements. At least, I (as a probable future user of your api) wouldn't be astonished by such a behavior.

Also, as you point out, having the pre-conditions upfront is more readable.

So second approach for me.

Answer 2

Your question, and example, raise a number of issues.

Do you really want to throw?

The Scala standard library goes to some length trying to accommodate whatever the client passes as index arguments. In many cases a negative index is interpreted to mean zero and anything beyond the collection size is simply ignored. See drop() and take() as examples.

That being said, if you're going to scold the client for bad argument values then it might make sense to test all received arguments even if one or the other becomes immaterial to the result.

assert() or require()?

assert() has some advantages over require() but the latter seems more appropriate for the usage you've got here. You can read this for more on the topic.

Reinventing the wheel.

Scala already offers a dropElements method, it's called patch.

def dropElements[T](array: Array[T], from: Int, howMany: Int) = {
  array.patch(from, Seq(), howMany)
}

Except that this returns ArraySeq[T] instead of Array[T]. Arrays, in Scala, can be a bit of a pain that way.

enhance-my-library

To make the new method look and feel more Scala-like you can "add" it to the Array library.

implicit class EnhancedArray[T](arr: Array[T]) {
  def dropElements(from: Int, howMany: Int): Array[T] = {
    arr.drop(from+howMany).copyToArray(arr,from)
    arr.dropRight(howMany)
  }
}

Array(1,1,1,8,8,7).dropElements(3,2)  // res0: Array[Int] = Array(1, 1, 1, 7)