CODEWITHSUNDEEP
c#.netexceptioncode-standards
Yuri Makassiouk
Yuri Makassiouk

Reputation: 557

Exceptions to control execution flow in managed .NET code - a good practice or not?

Here is a very simple functionality example, to make the discussion easy. Suppose, I am logging some abstract objects' content in a flat log. If an object's json representation contains property called EntityID with a GUID, I'd like to record it as a separate data element, if not - no matter, we still store the json but without the additional metadata.

So, I'll need a method which would return an ID if we are able to extract it or null if not. My instincts tell me to code it like this:

    private Guid? ParseSyncTableUniqueIdFromContent(string content)
    {
        try
        {
            object contentObject = JsonConvert.DeserializeObject(content);
            if (contentObject is JObject typedContentObject)
            {
                return Guid.Parse(typedContentObject.Value<string>("EntityID"));
            }
        }
        catch
        {
            // well, ok then
        }

        return null;
    }

The code in try section might fail in a number of ways: content might be null, it might be not null but cause some other exceptions inside DeserializeObject, and then even if we manage to deserialize, the object might not contain the property and even if it does it might not be parseable into a GUID. Since I don't care which problem would prevent me from getting to the value, I simply catch anything that might happen and move on.

An alternative would be to introduce several validations which would just return a null immediately.

if (string.IsNullOrEmpty(content))
    return null;

etc.

My sense of aesthetics and my habits say that a catch-all in this case is acceptable. But now let's remember that we are inside the managed .Net code and that makes me wonder. Does the exception mechanism become a resourse drain, then? How significant is the overhead on handling the exception in .Net?

The reason I ask is actually this. This particular function is quite often called within a large, high-load application, which employs an (in my opinion not well justified) method of catching all of the exceptions in the FirstChanceException event and tracking them all in Application Insights (I do think it's too blunt, but that's a different story for a different time, I guess). But it makes me think of such use of exceptions in .Net - is it too "expensive" even without such detailed logging taken into account? Is it (much) more efficient to write validation checks? (IMO, would suck if it is like that, but I feel like I need to know)

Upvotes: 1

Views: 872

Answers (1)

Heinzi
Heinzi

Reputation: 172320

Aside from the general recommendation of not using exceptions for control flow, the bigger problem of your code is that your error handler is much too general.

  • Your server ran out of hard disk space? -> Your method returns null.
  • You made a mistake when coding that causes an exception in rare edge cases? -> Your method returns null.
  • Anything else you didn't think of? -> Your method returns null.

In these cases, you want .NET to throw an exception and tell you about the problem - the more details, the better! Otherwise, you hide the real cause of the problem and start scratching your head why, suddenly, just on the customer's system, your method started returning null for perfectly valid input.

Thus, if you decide to violate the general rule and use exception handling for regular control flow (for which there can be valid reasons, but it should be a deliberate decision), at least make your exception handler catch as little as possible. In your case, that would be the specific exception thrown by JsonConvert.DeserializeObject when getting invalid input, and, in a separate try-catch block, the FormatException of Guid.Parse. (Better yet, replace the latter by Guid.TryParse, which is available in .NET Framework 4 and newer.)

Upvotes: 2

Related Questions