How could this code go wrong?

Perhaps one of the most important questions that any experienced developer asks is “How could this go wrong?”. The ability to reason logically about the myriad of ways that things can go wrong is a key skill of senior developers.

Consider this extension method, used to slice the first fragment from a string:

/// <summary>
/// Split a string a the first occurence of a given separator, 
/// returning both parts.
/// </summary>
/// <param name="original">Original string to split.</param>
/// <param name="separator">Separator to look for.</param>
/// <returns>The head and tail of the string.</returns>
public static (string head, string tail) SplitAt(
    this string original, char separator)
{
    var index = original.IndexOf(separator);
    return (original.Substring(0, index), 
        original.Substring(index + 1));
}

What could go wrong with this method?

One possibility is that the consumer might pass a null value for original, which would throw an uninformative NullReferenceException. We can protect against that by enforcing the method’s contract by adding a check at the start:

original.MustNotBeNull(nameof(original));

(I’m using an extension method to wrap the test and exception into a single line for brevity.)

More subtly, what should this method do if the supplied delimiter is not present in the supplied string?

As it stands, any developer who wants to understand what this method returns must have a detailed understanding of the string.Substring() method and how it behaves. If their understanding is correct, they’ll know what the SplitAt() method does. If their understanding is incorrect, their understanding will be wrong.

Relying on such a detailed level of understanding of the framework might be a mistake - it is making the presumption that any future developer either already knows these details or has the time to dig in and learn them. What if the developer using SplitAt() is inexperienced? What if they are very experienced, but not in the .NET technology stack?

If we’re instead very explicit about the behavior when the delimiter is missing (that we return the entire string as head and an empty string as tail), there’s no possibility of confusion. Let’s add the following code:

if (index == -1)
{
    return (original, string.Empty);
}

This also makes it clear that we avoid any extra allocations by returning the existing string; it’s not clear from inspection that string.Substring() returns the same instance when it’s possible to do so (reading the source reveals it does).

A more subtle thing to consider in situations like this is whether the extant behavior we’re relying upon is explicitly documented or an accidental side effect of the implementation. Most experienced developers have stories to share of situations where code reliant upon implementation details was broken by an update.

We’ve now improved this method. SplitAt() now complains appropriately when it’s used incorrectly. We’ve also made the behavior explicit so that readers don’t need to guess.

It turns out, however, that the question of How could this go wrong? is far more widely useful.