views:

173

answers:

5

I'm performing some parameter validation in a method and throwing an exception where necessary. Do I need to bother manually throwing exceptions of this type? As long as the caller is wrapped in a try..catch block a similar exception is thrown regardless of whether the manual checks are in place.

    public static Int16 ToInt16(this byte[] value, int startIndex, bool isBigEndian) {

        // are these exceptions necessary?
        if (value == null) {
            throw new ArgumentNullException("value");
        }

        if ((startIndex + 1) >= value.Length) {
            throw new ArgumentOutOfRangeException("startIndex");
        }    

        return (isBigEndian)
            ? (Int16)((value[startIndex] << 8) | value[startIndex + 1])
            : (Int16)((value[startIndex + 1] << 8) | value[startIndex]);
    }

This is an extension method used for converting 2 bytes in an array to an Int16 with a way to toggle Big Endian or Little Endian conversion.

+2  A: 

If you look at DotNET Framework code (say, using Reflector) then you'll find that the MS guys do throw these exception manually.

If you're making an SDK that will be heavily used by 3rd party programmers I recommend you throw ArgumentNullExceptions at least on top-level functions.

David Rutten
+9  A: 

The value of throwing exceptions here is that rather than getting a null deref exception you're going to get an argument exception that most importantly tells you the argument that is invalid. The exception is also more explicitly pointing to the cause than a generic null deref which could be anything.

Given that this is an extension method which is already one level of indirection I think it's probably more useful to be complete and include the exceptions. Especially given that you've obviously already written it, but even as a matter of policy. If it weren't an extension method I could go either way. I might also use an assert instead depending on how buried the code was.

I don't know about presently but in the .Net 1.1 and 1.0 Frameworks, the JIT compiler would never inline a call that had a throw in it. To avoid this would often mean creating a different method which would throw the exception itself.

By the way you have an off by one error with startIndex + 1 > value.Length; it should be >= instead.

Peter Oehlert
+5  A: 

As per the Class Library Design Guidelines, you should validate all parameters of public members of your types. http://msdn.microsoft.com/en-us/library/8ey5ey87(VS.71).aspx


UPDATE: A newer version: http://msdn.microsoft.com/en-us/library/ms229007.aspx

Alfred Myers
+2  A: 

Hi,

My opinion is that you should throw an exception. My thought is that you seem to be building reusable code that could become part of a library that is used in other programs or other parts of this program. While you might be using appropriate exception handling in your current code, there is no guarantee that you or someone else who uses this method in the future would do proper exception handling. Another consideration is that in maintenance, the implementation of this method could change and you have to think about what the implications would be of not catching the problem and throwing the exception immediately. I think that the code you have above is okay, except that I usually provide a more descriptive message to be just a little more explicit (of course that's my preference and what you are doing still communicates the problem).

Joe

+2  A: 

Any time you're writing a public method, you should treat it like a black box because other programmers will.

If you had two methods, MethodA(int i) and MethodB(int i), which is easier to understand?

  • MethodA(1000) throws an ArgumentOutOfRangeException
  • MethodB(1000) throws an IndexOutOfRangeException

MethodA is easier to fix without having to open the code for the method. MethodB might be throwing that exception for reasons unrelated to the argument, so you'd have to debug into the method to determine what's going wrong.

Cameron MacFarland