7

Sometimes I have this situation when it's quite easier to wrap whole piece of code in try-catch block rather than do a lot of checking which violently reduce code readability. For example, this

var result = string.Empty;
if (rootObject != null)
{
    if (rootObject.FirstProperty != null)
    {
        if (rootObject.FirstProperty.SecondProperty != null)
        {
            if (!string.IsNullOrEmpty(rootObject.FirstProperty.SecondProperty.InterestingString))
            {
                result = rootObject.FirstProperty.SecondProperty.InterestingString;
            }
        }
    }
}

I really prefer to do like this

var result = string.Empty;
try
{
    result = rootObject.FirstProperty.SecondProperty.InterestingString;
}
catch { }

But after code review I often hear from my mentor that I should avoid try-catch blocks when it's possible to make simple checking. Is it really so critical and each try-catch block eating a lot of system resources (relatively)? Is this resources used only when error raised or each case (successfull or not) is equally "heavy"?

Vitalii Korsakov
  • 45,737
  • 20
  • 72
  • 90
  • 1
    Just create a test case where you use TryParse & Parse+trycatch to parse an int. – L.B Jan 16 '12 at 22:46
  • Why does this function accept a `rootObjectType` at all? If it only accepted a `SecondPropertyType` the rest of the `ifs` could be elsewhere and your code could be much more testable. – Richard Szalay Jan 16 '12 at 22:46
  • 2
    @SteveWellens - I'm assuming that the `catch {}` is example code only. – ChrisF Jan 16 '12 at 22:46
  • See also: [Magic Argument Null Checking](http://msmvps.com/blogs/jon_skeet/archive/2009/12/09/quot-magic-quot-null-argument-testing.aspx) – George Duckett Jan 16 '12 at 22:57
  • Performance hit is your least problem when writing code like this :) There is a much more complicated but more proper way, but again is very slow. Look for some null dereferencing code on google, but you'll be back to != very soon :) Also, to support my story here's an [answer from an authority](http://stackoverflow.com/a/4244269/187955). – Nikola Radosavljević Jan 16 '12 at 22:57
  • @SteveWellens but what the difference between catch {} and when I don't make any actions, just checking if values are not null? – Vitalii Korsakov Jan 16 '12 at 23:01
  • 2
    @VitaliiKorsakov - the point is that you shouldn't be catching **all** exceptions, just the ones you know your code will raise. In this case `NullReferenceException`. – ChrisF Jan 16 '12 at 23:03
  • @ChrisF, not sure you should assume that... – Tony Hopkinson Jan 16 '12 at 23:04
  • @TonyHopkinson - I realise that now :) – ChrisF Jan 16 '12 at 23:04
  • What's wrong with this approach? – Vitalii Korsakov Jan 16 '12 at 23:19
  • 2
    @VitaliiKorsakov "but what the difference between catch {} and..." Catch {} is like disconnecting the smoke detectors in you children's bedroom. Catch {} is like putting black tape over the idiot lights on your car's dashboard. Catch {} can potentially hide the fact that something is horribly wrong. It can make debugging a nightmare. – Steve Wellens Jan 17 '12 at 02:42

9 Answers9

18

Whether exceptions are heavyweight or lightweight is completely irrelevant. A thrown exception that could have been easily prevented is a bug. Do not catch the exception: fix the bug so that you don't have to.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • 1
    But it's not a bug. It's just my attempt replace checking with error handling which provide more readability. The only problem is bad readability with simple checking. Can't you imagine situation when program has enough system resources to raise over 1000 exception a minute and it will not affect UI at all? – Vitalii Korsakov Jan 16 '12 at 23:39
  • 4
    @VitaliiKorsakov: You're missing my point. I'll say it again: **whether exceptions are lightweight is irrelevant. Treat all avoiable exceptions as bugs**. That is the right and proper attitude to have towards exceptions: they are *exceptional* and indicate that something is *wrong*. You should be able to turn on "break on all handled exceptions" in the debugger and *every single time the debugger breaks*, that's a bug. – Eric Lippert Jan 17 '12 at 00:17
  • @EricLippert, so true! I do work on a product that has so many places that exceptions are thrown that you can't use the "break on exceptions" feature at all. – Joel Jan 17 '12 at 03:01
  • 1
    While I agree that any `NullReferenceException` is indeed a bug, I think this answer is a bit too radical. There is some code, where exceptions make it much shorter and cleaner. For example with parsers it's much easier to abort the whole parse with a simple exception, instead of manually propagating the error. But the user telling you to load an invalid document should be rare, but is no bug. Pity that C# doesn't have an alternative error handing facility for such code, so one has to choose between exceptions and manual error propagation. – CodesInChaos Jan 18 '12 at 10:35
  • 2
    @EricLippert 'You should be able to turn on "break on all handled exceptions" in the debugger and every single time the debugger breaks, that's a bug.' -- this can be impossible when using third-party libraries; some are poorly designed with respect to exceptions. Another classic example is I/O, where you may check that a file exists before reading, but that can be foiled by another user deleting the file between the check and the actual reading of the file. – phoog Jan 18 '12 at 19:58
  • 2
    @phoog: As for your first point: the existence of other people's buggy libraries is a good reason to not make the problem worse. Start reporting bugs to them. As for your second point: exogenous exceptions as you describe are *unavoidable*, but are still *exceptional*. The odds of such a thing happening accidentally while debugging are quite small. – Eric Lippert Jan 18 '12 at 20:23
  • 1
    @EricLippert I agree with you in principle, but poor design is not the same as bugginess. Perhaps I was unclear. The example I have in mind is a web service that throws exceptions for non-exceptional situations like "we couldn't find an object with the ID you specified". It's unfortunately unlikely that the authors of this system would redesign it just so I can debug my code with "break on all handled exceptions" enabled. – phoog Jan 18 '12 at 20:38
9

Exceptions in the .NET framework are relatively heavy - it's worth going to a bit of effort to avoid them. After all, they're called Exceptions - they shouldn't be common.

That said, they are nowhere near as expensive as some people seem to think.

When debugging under Visual Studio, processing an exception can take a few seconds as the IDE brings up the line at which the exception was caught. Because of this, some people think that every exception takes a few seconds to process and that they must therefore be avoided at all costs.

I've seen people blame poor system performance on the fact that it's throwing a few dozen exceptions an hour.

This is a myth. The system is perfectly capable of throwing and catching many thousands of exceptions per second.

That said, you can tidy up your original code with a couple simple extension functions like this:

var result
    = rootObject.With( r => FirstProperty)
          .With( r => r.SecondProperty)
          .Return( r => r.InterestingString, string.Empty);

Where With has this definition:

    public static TResult With<TInput, TResult>(
        this TInput o, 
        Func<TInput, TResult> evaluator)
        where TResult : class
        where TInput : class
    {
        if (o == null)
        {
            return null;
        }

        return evaluator(o);
    }

And Return has this:

    public static TResult Return<TInput, TResult>(
        this TInput o, 
        Func<TInput, TResult> evaluator, 
        TResult defaultValue) 
        where TInput : class
    {
        if (o == null)
        {
            return defaultValue;
        }

        return evaluator(o);
    }
Bevan
  • 43,618
  • 10
  • 81
  • 133
  • +1, was trying to search for this (extension method) but couldn't find it anywhere. – George Duckett Jan 16 '12 at 23:01
  • Excelent! Almost what I need! P.S. I think _null_ is unnecessary when using _With_ extension – Vitalii Korsakov Jan 16 '12 at 23:35
  • 1
    This is a good idea for a solution, but the term `With` doesn't say to me "don't evaluate if null". It makes sense when coupled with `Return` but doesn't when seen initially on its own (e.g. when someone is first "discovering" it via intellisense). Can you give it a better method name? Some related discussion to this solution - http://stackoverflow.com/questions/4244225/c-sharp-if-null-then-null-expression and http://stackoverflow.com/questions/4244225/c-sharp-if-null-then-null-expression - also, I'd recommend jamming a `default(TInput)` in there somewhere :) – Merlyn Morgan-Graham Jan 17 '12 at 20:45
  • 2
    The name *is* a challenge - I've found that there are a whole class of methods whose names don't fit well with the old Verb/Noun conventions of a decade ago. I don't have anything better than `With()` - a descriptive name like `EvaluateIfNotNull()` introduces so much noise that it's worse than the problem we're trying to cure. – Bevan Jan 18 '12 at 04:04
  • Oops, posted the same link twice. Here's the 2nd one I meant to post: http://stackoverflow.com/questions/1917531/can-i-force-my-own-short-circuiting-in-a-method-call – Merlyn Morgan-Graham Jan 21 '12 at 00:17
5

Exception handling

I wouldn't worry about "how heavy" exceptions are in code like this. I'll explain why later.

Exceptions are for exceptional situations. Having an exception handler means you expect those properties never to be null.

When to write guard conditions

Is it common for those properties to be null?

If so, this is not an exceptional situation. You should write the appropriate null tests, and change your exception handler code into null-condition code.

Is it uncommon for those properties to be null, i.e. can you reasonably expect that they would never be null?

If so, you could simply avoid writing the null checks, and just let the underlying code throw. However you won't get a lot of context as to which property threw an exception.

You could also do null checks and throw an exception that is more context specific.

When to write an exception handler

If it is uncommon for those properties to be null, then this is an exceptional situation. But that doesn't mean you should necessarily have a handler.

Do you have an easy way to test for the exceptional situation before it occurs?

If so, then you should test for this before allowing the underlying code you're using to throw an exception. Since you simply have to check for null, I'd say this is pretty easy.

Do you have reasonable logic to handle this case at this level?

If you have a reasonable way to deal with the exceptional situation at that level, and still guarantee that your method executes correctly, then go ahead and add the handler code. If you're relying on a mechanism like returning null, then be very certain that it makes sense from the consumer's perspective that they won't always get a result. E.g. name the method FindInterestingString rather than GetInterestingString.

If you don't have a reasonable way to deal with the situation, do not place the exception handler at that level. Let your exception bubble up and handle it at a higher spot in the code.

If you don't have a reasonable way to handle an exception at all, just let the program crash. This is always better than swallowing an exception and continuing. That hides bugs.

Exceptions to these rules

Sometimes you can't easily test for a condition without throwing an exception.

External dependencies such as the filesystem will change underneath your program. Even if you do a pre-test, and even if the pre-test passes, an exception might be thrown as soon as you try to use the object. In cases like this, you can't do anything about it, and must rely on exception handling.

Complex validation like e-mail addresses and URIs might require you to use a construct that throws an exception. Then again, it might not. You should always look for the most appropriate way to do error handling that matches your intent. Only bend to use exception handling when you have to.

Performance

Performance is very unlikely to be an issue in error detection code.

Performance matters in high-use code (when you're writing a framework), in bottlenecks in your application, and in algorithms that are known to be CPU/memory intensive. You should learn when to worry about perf, but it should always be a secondary concern to readability, maintainability, and correctness of your code.

You'll find it is impossible to perfectly predict what will become a performance problem in your whole application. The only way to get an accurate picture is to run your code with realistic scenarios in realistic conditions and profile it. Until you get to this point in developing an application you shouldn't worry about perf except in cases where you know it will be an issue.

Using exceptions doesn't come with as high a perf cost as many people might have you believe. In .Net, they're designed to perform very well when an exception isn't thrown. This jives with the fact that exceptions are for exceptional situations.


Your code samples

There were a few additional issues with the code samples you provided. Hopefully I can point some of those out before you get too trapped by them. If not, hopefully you can look back to these for guidance when you run into problems.

Writing exception handlers

The code you wrote for your exception handler was not acceptable at all. Here's some guidance on writing better exception handler code:

Bad:

try
{
}
catch // Note: Doesn't catch `Exception e`
{
    // ... eats the exeption
}

This is bad form and should never be used. There is absolutely no way that you're handling all exception types correctly. The most commonly used example is an OutOfMemoryException.

Possibly acceptable:

try
{
}
catch(Exception e)
{
    logger.Log(e.ToString());
    // ... eats the exeption
}

If you catch the exception and log it or display it, it might be okay to eat the exception. This is only okay if you are actively monitoring/reporting these exceptions, and have a way to guarantee that these exceptions will be diagnosed.

Okay:

try
{
}
catch(Exception e)
{
    logger.Log(e.ToString()); // Make sure your logger never throws...
    throw; // Note: *not* `throw e;`
}

// Or:

try
{
}
catch
{
    // Todo: Do something here, but be very careful...
    throw;
}

You can do whatever you want in an exception handler if you're very careful not to create new exceptions, and if you re-throw the exception. This will guarantee that the error gets noticed. If you re-throw an exception, make sure to use throw; rather than throw e;, otherwise your original stack trace will be destroyed.

Good:

try
{
}
catch(NullReferenceException e)
{
    // ... Do whatever you want here ...
}

This is safe because you are only catching certain exception types that are known to be thrown by the code in the try block. It is easy to understand the intent of the code, and easy to code review. It is easy to understand whether the exception handler code is okay or not okay.

Avoid duplicating code

Never re-access properties when you can avoid it. Instead of writing code that accesses your properties like this:

rootObject ...
rootObject.FirstProperty ...
rootObject.FirstProperty.SecondProperty ...
rootObject.FirstProperty.SecondProperty.InterestingString ...

...call the getters only once:

var firstProperty = rootObject.FirstProperty;
var secondProperty = firstProperty.SecondProperty;
var interestingString = secondProperty.InterestingString;

Your code sample will look more like this:

if (rootObject != null)
{
    var firstProperty = rootObject.FirstProperty;

    if (firstProperty != null)
    {
        var secondProperty = firstProperty.SecondProperty;

        if (secondProperty != null)
        {
            var interestingString = secondProperty.InterestingString;

            if (!string.IsNullOrEmpty(interestingString))
            {
                result = interestingString;
            }
        }
    }
}

One reason to do this is that the getters might have complex logic, and invoking it multiple times could cause a performance impact.

Another reason is that you avoid repeating yourself. Code is always more readable when it doesn't have a lot of repetition.

When you repeat yourself, maintainability is also impacted. If you change the name of one of those properties, you're going to have to change each line of code that it occurs in, which will make it harder to reason about the impact of the change.

Avoid digging too deep into a dependency hierarchy

You should avoid chained property access within the same method. I.e.:

rootObject.FirstProperty.SecondProperty.InterestingString

Even if you've split it up to avoid repeating yourself (like I suggested above), you still probably haven't factored your code correctly. Your code is still tightly coupled to the hierarchy of that data structure. Any time you change that hierarchy, any code that traverses that hierarchy will need to be changed. If this is all your code, then you're in bad shape.

To avoid this, separate the code that knows about each level from the levels below it.

Code that handles the root object should only call code that handle objects directly below the root. Code that handles the FirstProperty should only know about properties at the SecondProperty level (under FirstProperty). The only code that should know anything about InterestingString is handler code for the object type returned by SecondProperty.

An easy way to do this is to split your traversal code up, and move it into the objects themselves.

See:

Example code splitting up the logic:

public class SomeClassUsingRoot
{
    public string FindInterestingString()
    {
        return root != null
            ? root.FindInterestingString()
            : null;
    }

    private RootSomething root;
}

public class RootSomething
{
    public string FindInterestingString()
    {
        return FirstProperty != null
            ? FirstProperty.FindInterestingString()
            : null;
    }

    public SomethingTopLevel FirstProperty { get; set; }
}

public class SomethingTopLevel
{
    public string FindInterestingString()
    {
        return SecondProperty != null
            ? SecondProperty.InterestingString
            : null;
    }

    public SomethingLowerLevel SecondProperty { get; set; }
}

public class SomethingLowerLevel
{
    public string InterestingString { get; set; }
}

This isn't the only way to solve the problem. The key is to split up the logic that handles each level into separate methods, or (even better) separate objects. This way you have a smaller impact when the hierarchy changes.

Community
  • 1
  • 1
Merlyn Morgan-Graham
  • 58,163
  • 16
  • 128
  • 183
2

Exceptions are for.. well, exceptional situations. They are for when something occurs that cannot otherwise be planned for. Exceptions have a certain amount of overhead, and using them to catch general problems like this is considered bad practice, particularly if you are just ignoring the result of the exception (with your empty catch block).

They may make your code LOOK cleaner, but they don't make your code EXECUTE cleaner.

Erik Funkenbusch
  • 92,674
  • 28
  • 195
  • 291
  • Sorry, I didn't get last sentence. What did you mean? – Vitalii Korsakov Jan 16 '12 at 23:08
  • @VitaliiKorsakov - Exceptions are not clean. There's a lot of work that goes on under the scenes. On top of that, what happens if a different exception occurs in one of your blocks, your catch all will ignore it and you will have bugs that are hard to track down. Using exceptions for this is poor practice, and you will get criticized by any professional developer. – Erik Funkenbusch Jan 17 '12 at 01:00
  • Exceptions are a form of goto. While they have their place, you don't want to be using them willy-nilly. – Steve Wellens Jan 17 '12 at 03:02
1

It depends.

If rootObject etc. are likely to be null then coding it the first way is better as it's not an exceptional circumstance that it is. However, it will make the execution of the method slightly slower. Though there are ways of recoding the nested if statements to avoid deep nesting and allow for quick exits from the method.

On the other hand if normal execution speed is an issue and rootObject etc are unlikely to be null then coding it the second way is better as it **is and exceptional circumstance.

You need to analyse your system to see which is the better way for your application.

ChrisF
  • 134,786
  • 31
  • 255
  • 325
  • "However, it will make the execution of the method slightly slower." I understand that, but what if try-catch block placed in UI where performance is not so critical? – Vitalii Korsakov Jan 16 '12 at 23:05
  • @VitaliiKorsakov - By how much depends on all sorts of factors and in fact might not be measurable. Plus that statement refers to the defensive programming version, not the exception handling version. – ChrisF Jan 16 '12 at 23:07
1

One way to help readability is to reverse your conditions:

var result = string.Empty;
if (rootObject == null) return result;
if (rootObject.FirstProperty == null) return result;
if (rootObject.FirstProperty.SecondProperty == null) return result;
if (!string.IsNullOrEmpty(rootObject.FirstProperty.SecondProperty.InterestingString))
{
    result = rootObject.FirstProperty.SecondProperty.InterestingString;
}

The next step is to use the conditional shortcutting the compiler will do for you:

var result = string.Empty;
if (rootObject == null || rootObject.FirstProperty == null ||
    rootObject.FirstProperty.SecondProperty == null) return result;
if (!string.IsNullOrEmpty(rootObject.FirstProperty.SecondProperty.InterestingString))
{
    result = rootObject.FirstProperty.SecondProperty.InterestingString;
}
Evan Powell
  • 960
  • 5
  • 4
1

One option would be to use code contracts. They are a very clean way to doing the type of checking you are doing, and if you configure your debug build properly the compiler can actually find code that would violate your contracts. An empty catch block is really not a good idea (and not because it would use resources...it is just not good coding for lots of reasons).

Mike Hanrahan
  • 1,192
  • 8
  • 18
0

In most of the cases you should not test for references to be not null because null should not even be in the range of possible values then. If a function can not return something in the range of possible values then this function has no other choice than throwing an exception. This should happen without explicit null check. It should happen when the return value is being constructed for e.g.

But this means that you need non nullable types, something the language should ideally support but C# does not.

You can use something like NonNullable<> from Jon Skeet.

Patrick Fromberg
  • 1,313
  • 11
  • 37
0

If you can prevent your code from throwing an exception in the first place, than you should do so.

Try/Catch blocks are good to use when they're used right(such as when you're accessing something that is outside your code's control like opening network connections).

You can also use them to continue running your code if you deem an error in a certain part of your code non-fatal. Just make sure you properly handle the error in your catch block.

One thing you should think about, however, is how the compiler executes your predicates.

It will take the leftmost clause of your predicate, rootObject != null, and if that's false, and it's AND'ed to your other clauses, than that predicate is guaranteed to evaluate to false. The compiler will then ignore the rest of the predicate, therefore you can do the following:

if (rootObject != null && rootObject.FirstProperty != null && rootObject.FirstProperty.SecondProperty != null && !string.IsNullOrEmpty(rootObject.FirstProperty.SecondProperty.InterestingString))
{
    result = rootObject.FirstProperty.SecondProperty.InterestingString;
}