Should I throw guard exceptions that will be thrown by internal methods anyway? [duplicate]

Question

I have some fairly simple code (C#):

    /// <summary>
    /// Truncates a string to a maximum length. 
    /// </summary>
    /// <param name="value">     The string to truncate. </param>
    /// <param name="maxLength"> The maximum length of the string. </param>
    /// <returns>
    /// The original string if its length is less than or equal to max length, else a truncated
    /// string of length maxLength.
    /// </returns>
    public static string Truncate(string value, int maxLength)
    {
        if(maxLength <= 0) throw new ArgumentOutOfRangeException("maxLength");

        if (string.IsNullOrEmpty(value)) return value;

        return value.Length <= maxLength ? value : value.Substring(0, maxLength);
    }

It's clear that if maxLength is less than 0, I can't use it as a string length, and so I am currently using a guard statement to prevent invalid maxLength values.

My question is: is it good or bad practice to use this guard statement, when value.Substring will throw the same exception anyway? Is it just wasted code?

Answer 1

Generally the purpose behind throwing the exception yourself is to provide your own personal exception. For example, if Truncate were a method available to use in a library, lets call it ABC, it might be nice to have all your exceptions fall under a base exception called ABCException which also comes from the library. In other words, for any problems caused by library ABC, you can optionally catch it using the base exception ABCException. This is extremely useful to identify problems coming from a specific library.

Another reason to perform your own check is for a "sanity check". In other words, yes, perhaps Substring throws this exception, but should you decide tomorrow that something should be done prior to performing the actual Substring call, your code may throw a different and uncaught exception instead. This may make debugging the problem that much more difficult since you may discard the possibility of the problem being in Truncate because of the fact that Truncate is supposed to throw another exception in such circumstances. Call it what you will, but I like to call it defensive programming. Is this likely? No, but if it is only going to cost you 10 seconds to add, I'd say to go ahead and add it.

Note that I wouldn't add checks like these unless they were public methods and therefore available to be called by anyone. Also, in circumstances in which the method is specifically documented as such, I wouldn't even perform checks. If the caller fails to respect the documentation for said method, it is the caller's responsibility, not yours.

Also, if I may make a minor optimization, the contents of the if statement should be the most likely scenario rather than the least likely. That said, I would rewrite it:

public static string Truncate(string value, int maxLength)
{
    if(maxLength > 0 && !string.IsNullOrEmpty(value)) 
        return value.Length <= maxLength ? value : value.Substring(0, maxLength);
    else
        throw new ArgumentOutOfRangeException("maxLength");
}

Hope that helps!

Answer 2

It comes down to the contracts that your methods obey.

Your example code doesn't show any exception specifications, so technically nobody should assume anything about what happens for maxLength < 0 - it doesn't even matter whether you throw an exception at all.

If you do want that behaviour to be part of your semantics, it should be documented. Then whether or not to throw the exception yourself comes down to what the contract of the built-in method is. It that one is guaranteed to throw this exception, you can simply rely on that (although some would argue that throwing the exception explicitly is still better because of readability and fail-early concerns). If it isn't, you must throw the exception yourself.