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.