Before everyone espouses code correctness, I realize that the generally correct way to do things is to not use try/catch for control flow. However, this is a bit of an edge case, and I'd like some input on what others would do in this situation.
This is example code, not actual code, but it should get the point across. If anyone wants me to define aDictionary or some mock classes for the types here just let me know and I will.
public bool IsSomethingTrue1(string aString)
{
if (aDictionary.ContainsKey(aString)) //If the key exists..
{
var aStringField = aDictionary[aString].Field; //Get the field from the value.
if (aStringField != null) //If the field isn't null..
{
return aStringField.SubField != "someValue"; //Return whether a subfield isn't equal to a specific value.
}
}
return false; //If the key isn't found or the field is null, return false.
}
public bool IsSomethingTrue2(string aString)
{
try
{
return aDictionary[aString].Field.SubField != "someValue"; //Return whether the subfield isn't equal to a specific value.
}
catch (KeyNotFoundException) //The key wasn't in the dictionary..
{
return false;
}
catch (NullReferenceException) //The field (or the dictionary, if it is dynamically assigned rather than hardcoded) was null..
{
return false;
}
}
So, in the above example the first method checks to see if the dictionary contains the key, then if it does, it checks to see if a field is null, and returns true if a subvalue isn't equal to a specific value, otherwise it returns false. This avoids try/catch, but every single time the code is accessed it performs a check on the dictionary to see if the key is present (assuming no under-the-hood caching/etc. - let me know if there is any please), then a check to see if a field is null, and then the actual check we care about.
In the second method, we use try/catch and avoid multi-tier control logic. We immediately go "straight to the point" and try to access the value in question, and if it fails in either of the two known ways it will return false. If the code executes successfully, it may return true or false (the same as the above).
Both of these methods will get the job done, but my understanding is that the first method will do so more slowly on average if nothing goes wrong in most cases. The first method will have to check everything every time, where the second only has to handle cases where something goes wrong. There will be an extra space for an Exception on the stack, and some instructions for jumping to the different catch blocks/etc.. This should all not affect performance in the regular case where nothing is wrong other than that one stack variable, however, if I understand correctly what I've read here: Do try/catch blocks hurt performance when exceptions are not thrown?
Of course with this exact example, the difference will be negligible - however, imagine a complex scenario with a lot of checks in a complex if/then/else tree compared to a try/catch with a list of failure conditions. I realize that yes, this sort of code could always be broken down into smaller bits or otherwise refactored, but for the sake of argument let's say it can't be changed other than the control flow (there are cases where changing actual logic would require a re-validation of scientific algorithms, for example, which is costly/slow and needs to be minimized).
Textbook I realize that the answer is to use the first method, but in some cases the second method could be significantly faster.
Again, I know this is somewhat pedantic, but I really like to make sure I write the most efficient code possible in places where it counts, while keeping everything readable and maintainable (and well-commented). Thanks in advance for providing me with opinions on the matter!
You seem to agree that not using boneheaded exceptions to control program flow is a proper thing to do.
Nonetheless you'd still prefer the try-catch solution because it is a concise and relatively clear way, and because it may be even faster due to absent null/presence checks and that exception throwing and handling may be not that bad performance-wise.
It is all a good reasoning, but there are few moments we should be more careful about:
With anything related to performance there is only one thing you can be (relatively) sure - benchmarks. Measure everything.
We do not know exact patterns of data in dictionary. If nothing can throw exceptions during property drilling, then you may well be better with exceptions.
But cost of actual null comparisons/TryGetValue methods is minuscule compared to exception throwing.
Again - measure it against sample data, consider how often this code will get executed and only then make any decision on acceptable/unacceptable performance.
No one would argue that this code is shorter than the one with a lot of ifs. And it is much more difficult to make any mistakes with it.
But there is one fallacy hidden behind all this readability - such try-catch code is not fully equivalent to the original.
Why isn't it fully equivalent?
Dictionary<String, T>, but rather some custom IDictionary that may have a defect, that causes NullReferenceException during internal calculations. T object may be a proxy object with internal object, that under certain cicumstances starts initialized with null, causing NullReferenceException in T.SomeProp.T can be a proxy to some object in another Dictionary, it is actually absent from, hence KeyNotFoundException.In all those cases we will ignore a potential defect.
Isn't it a movie plot threat that won't happen in your particular case, you may say? Perhaps, or perhaps not. You may decide to accept such risks as rather tiny, but such cases are not impossible, because the system will change, while this code won't.
All in all, I'd rather stick to the first noexcept solution. It is a bit longer, but it does exactly what it says, in any case won't have significantly worse performance, and doesn't throw exception for no reason.