Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
    
related: What is the advantage of wrapping exceptions –  gnat Sep 2 at 12:47
1  
Why are you disallowing a truncate to 0 length? –  Pieter B Sep 2 at 13:38
    
It's a requirement from the customer. Hmm, not sure why that's not in the docs, thanks for spotting it. –  Nick Udell Sep 2 at 13:49
    
A more interesting question might be "Should I write methods that re-implement and/or just wrap standard class library methods?". 'cause the answer would be "No." –  Ross Patterson Sep 6 at 0:41
    
Eventually, they want some additional error handling logic in there that notifies a WCF duplex (which cannot serialize Exceptions sadly, so I'd also have to support an events based service), so it seemed a DRYer approach to encapsulate all of that in a method now, although that might be breaking YAGNI. –  Nick Udell Sep 8 at 8:11

2 Answers 2

up vote 0 down vote accepted

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!

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.