5

For example lets take this method:

public List<string[]> ReadAll(int listCapacity)
{
    List<string[]> list = new List<string[]>(listCapacity);

    while (Read())
    {
        list.Add(GetCurrentRow());
    }

    return list;
}

If listCapacity is less than zero, an ArgumentException will be thrown by the constructor of List<T>. Does it make sense to double check this? Passing the argument to List<T> immediately seems careless, but checking it seems silly because List<T> will definitely check it.

Important note: the code snippet is not just a copy-paste method, its part of a full utility.

11

Aside memory allocation reason (which, by the way, looks like premature optimization to me), there are other elements in favor of catching invalid arguments as soon as possible:

Stack trace

When an exception occurs, you expect the few last lines to show you the location of a mistake. Having to search from bottom to top of the stack to find the culprit will waste your time later, during debug, when you specifically won't have time.

Being sloppy about input validation and letting called methods handle that again and again will mean that the stack trace will be larger than it should be. If you know that ReadAll cannot accept a negative capacity, why adding a line to stack trace, instead of throwing an exception right now in order to make the debugging easy?

Readability

Your code is expected to be self-documenting. I can only know your method doesn't accept negative values if I inspect the method closely. This is OK if I'm about to modify the method; this is not OK if I simply want to use the method, and don't really care how is it implemented.

And no, I can't rely on documentation, since it's always incomplete and obsolete.

Validating the inputs right at the top of the method would help readability: it would be clear that I can't use negative values, and I would avoid wasting additional time:

  • Inspecting the body of the method,
  • Inspecting every method in the code base this method calls,
  • Reading MSDN for every .NET Framework method this method calls.

Code contracts

Since you tagged your question and your code looks like C#, I can't avoid talking about code contracts. If you have used code contracts, you would be forced to add a Contract.Requires anyway, otherwise the static checker will complain that nothing guarantees that the list constructor will be called with valid arguments.

That's also the benefit of code contracts: they force you to handle invalid inputs at the top of the stack, so when the input is invalid, you stop right now, instead of calling dozens of other methods before encountering an exception.

9
  • The additional check is only effective if an invalid argument is passed which should be uncommon, yet it requires some instructions. So I doubt that this has anything to do with optimization. I think you are right concerning debugging. It sounds plausible to me that any public method has the responsibility the check the arguments that it requires, because nothing implies that it is going to be passed along exclusively, maybe the method has some special treatment for negative values. Jul 4 '14 at 17:28
  • In general, your arguments are good. However, in this particular example, it doesn't require a rocket scientist to expect that listCapacity be non-negative. And, if List ever somehow changes to accept a negative capacity, so should your code (probably).
    – user949300
    Jul 4 '14 at 18:14
  • 2
    @user949300: "it doesn't require a rocket scientist to expect that listCapacity be non-negative": it looks like it should be non-negative, but only inspection of the method and MSDN confirms that. I stopped counting long ago the cases where, in programming, something obviously looked like something, but actually wasn't. Jul 4 '14 at 19:32
  • @MainMa Question: every time you allocate or insert into an array, do you check the size or index for >= 0? I doubt it. IMO, this case is similar.
    – user949300
    Jul 4 '14 at 19:44
  • 1
    As far as readability, listCapacity is a parameter, and the docs should say something like "used to allocate the List" and, if you are paranoid, "non-negative". IMO, that is better than a fairly pointless test.
    – user949300
    Jul 6 '14 at 15:54
3

The answer is: it depends. In your example, you should let List throw its exception and let any exception bubble up the chain.

Your example method should not know or care whether the value is invalid, since it's sole purpose is to pass it along to something else. If you were using the value in ReadAll as something other than an argument to List, you would be correct to test it before use, but you're not.

An invalid argument to your method is not one that will not be understood by a separate utility, it is one that does not make sense to your code.

As a side note, parameters that exist simply to be passed along, should be fairly rare.

4
  • Passing a negative intial list capacity definitely doesn't make sense, whether my external code is qualified to judge that, is a very good question. You have a good point. Jul 4 '14 at 17:05
  • @LeopoldAsperger: it doesn't make sense to you, but List could be changed so that negative values have meaning -- it could for instance be a flag that changes the way reallocation works. So that instead of doubling when necessary it only increases it by 50% or conversely by 3x.
    – jmoreno
    Jul 4 '14 at 17:29
  • What you're saying is definitely something to be concerned about overall. But I really doubt Microsoft would implement such hack, they would probably provide another overload. Jul 4 '14 at 17:31
  • @LeopoldAsperger: true, they are much more likely to simple ignore it if it's negative. Frankly I halfway consider it a bug that it throws an exception now.
    – jmoreno
    Jul 4 '14 at 23:16
2

Short answer: no, it's not OK. Because even calling the constructor of List means, from the compiller's perspective: memory allocation for the new object, writing references for it in the reference table, accessing other subroutines just to create the new List object, just to have it basically blow up in your face by throwing an exception.

I know it doesn't seem like much with all these available resources on the system, but it's still not something I would do - if I can avoid it with just one extra line of code.

6
  • 1
    This hits the nail on the head. Jul 4 '14 at 13:59
  • 3
    -1 I think that @MainMa has a much better argument. Efficiency is definitely NOT the major consideration here.
    – user949300
    Jul 4 '14 at 18:12
  • 1
    I don't think the answer implies that efficiency is a consideration, this is about correctness. @MainMa has a very informative answer which received my upvote, but I prefer this point of view. For example why permit a crash when not List<T>.ctor() but ReadAll() requires the argument. Jul 4 '14 at 18:30
  • Unlike MainMa, nowhere in this answer is the word "correct" or "contract" or "readability" or similar. All the terms are about memory, writing to tables, calling other subroutines, etc. This answer is all about efficiency.
    – user949300
    Jul 4 '14 at 19:40
  • 1
    @user949300 I think you're missing the point. The remarks about runtime mechanisms were explanatory, not judging. Jul 4 '14 at 20:36
1

Gotta disagree with the accepted answer. The argument about allocating memory, etc. is only relevant if this method is called often with an illegal argument.

The purpose of checking arguments (programming by contract) and throwing exceptions is primarily to fix bugs during development. During development, efficiency is not an issue. Combined with unit tests, this should assure that ReadAll() is not called with a negative argument. Or at least only called rarely in a case you didn't test.

So, with proper development and testing, efficiency will never be an issue. Letting the check slip one or two levels is fine. Though you may want to add a comment.

EDIT #1 added

Let's make the example slightly different. The function returns the result as an array:

public string[] ReadAll(int maxCapacity) {...}

Would you check for maxCapacity>= 0 in your code? Unlikely. IMO, this case of using a List is close enough to an Array that checking the initialCapacity is overkill.

EDIT #2 Added

There are many times where it is completely unreasonable to check an incoming argument yourself. For example, say that there is an XML String coming in. Your method looks something like this: (pseudo-Java-like code, nor am I claiming that this is a good design)

public double getFooBarFraction(String xml) throws AllKindsOfExceptions {
   Document doc = DOMParser.getInstance().parse(xml);
   return Double.parseDouble(doc.getElement("Foo")
                                .getElement("Bar")
                                .getAttribute("fraction"));
}
  1. Are you going to validate the XML yourself, or let the utility DOMParser do that?
  2. Are you going to look for elements and attributes yourself, or let Document do that?
  3. Are you going to test that count can be parsed to an double, or let Double do that?

The answer is almost certainly "no" to all of the questions. Now, you probably should wrap the calls in try/catch blocks for better error handling, but you still let the utility create the original exception.

In rereading the other answers, they do say "where it's easy to test" etc. I guess if you can do the test in one or two lines of code, consider doing it.

7
  • 1
    I agree with what you're saying, but you have no way of knowing that the behavior of the List class will remain unaltered forever. It may someday be that it won't throw an exception anymore, so what happens then with your (hopefully, already in production) code then if you depend on this behavior?
    – Thyamarkos
    Jul 4 '14 at 16:42
  • I'm still indecisive in this matter, but @Thyamarkos had a very good point. Favoring succesful initialization is logical and it is a good practise, even if its regarding a framework component such as List<T>. Jul 4 '14 at 16:58
  • @Thyamarkos If, somehow, List changes and allows a negative number of elements (unlikely I know!), your code should probably change to allow that. A more realistic scenario might be an upper limit on a List or Map or whatever. If the default upper limit increases in .NET version 34.0, because they come up with a better algorithm or memory is cheaper, your code should, most likely, accept that higher limit.
    – user949300
    Jul 4 '14 at 18:08
  • @user949300 - The current implementation of List<T> has no upper limit, but unlike negative values, such limit is not reliable at all since the limit will be an arbitrary power of 2 between 0 and int.MaxValue. Prior to doing an operation, there is no way to predict a OutOfMemoryException either, so I guess letting this slip could be justified in that sense. Jul 4 '14 at 18:52
  • 1
    @Thyamarkos: if the behaviour of the .NET framework List class would be changed by Microsoft in such a way, that would break compatibility for hundred thousands of programs - so I guess we can safely assume they will not change that.
    – Doc Brown
    Jul 6 '14 at 10:14

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.