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)

Answer 3

First of all, I'd recommend using require over assert for pre-conditions. Indeed require is intended for (and was probably specifically added for) pre-conditions, as described in the "Assertions" section in the documentation). Also, calls to assert could be elided if your code is compiled with -Xdisable-assertions. This might not be a big problem in this specific case, but if the implementation relies on certain pre-conditions being enforced, bad things could happen. If you (or the users of your function) will use a static analysis tool, it probably makes better use of the requires than of the asserts.

I'd prefer the second approach, for several reasons.

Strictly speaking, you can only enforce a pre-condition if you check it at the beginning. With the first approach your pre-condition is actually howMany >= 0 && (howMany == 0 || (0 <= from && from < array.length)) instead of howMany >= 0 && (0 <= from && from < array.length).

Why would you prefer the latter over the former? If the caller passes an out-of-bounds from, it's not unlikely that there is a bug in the caller's code. Admittedly I'm speculating here, as there are many potential use cases for your function. But if someone comes up with a convincing use case for passing out-of-bounds indexes that outweights the contrary arguments, you can still relax the precondition later. The other way around would break backwards compatibility.

You say that "if howMany is zero, it should not do any modification". That follows from the intended semantics of your function. Whether you implement it as an early-return or not is an implementation detail, so in my opinion the line if (howMany == 0) return; should be part of .... in your question and then the question answers itself in favor of the second approach.

Answer 4

I prefer killing two birds with one stone: avoiding return by putting the method body inside a single else clause, and putting all the assertions together. Also, assertions should fail with a helpful message if possible:

def dropElements[T](array: Array[T], from: Int, howMany: Int): Array[T] =
  if (howMany == 0) array
  else {
    assert(howMany > 0, "howMany must > 0")
    assert(from >= 0, "from must >= 0")
    assert(from < array.length, s"from must < ${array.length}")

    ...
  }

Answer 5

My 2 cents: in Scala, I usually try to avoid using that kind of if statements, with return(s) buried in the middle of a method call (sure, this method is short, but in longer ones... debugging gets difficult): there are cleaner, better ways to do it. Also, I'd be proactive and check if the array you get is null.

I would do something like this

def dropElements[T](array: Array[T], from: Int, howMany: Int): Option(array) match{
    case Some(arr) if arr.nonEmpty =>
        // here 'arr' is guaranteed to be non-null, and with at least one element
        require(howMany > 0, "howMany must > 0")
        require(from >= 0, "from must >= 0")
        require(from < array.length, s"from must < ${array.length}")
        // rest of the code here

    case _ => // do nothing if the array is null, or it is empty
}