Both examples you show have issues.
Example 1 violates DRY. You have three instances of the same "failure" outcome (in this case
ResetLine()). Imagine if the failure outcome requires multiple steps. Now imagine you need to change the failure logic. You'd have to make the same change three times, or you'd end up with unexpected behavior.
Example 2 solves the DRY issue, but it actually fails to properly null check. If
zoomEventArgs_ is null, you're going to run into a null reference exception when you try to access
zoomEventArgs_.focus. Note that the
as conversion returns
null when it cannot convert the object to the chosen type.
There is an easy fix for this:
if(zoomEventArgs_?.focus != null)
?. operator. It is the null propagation operator that will prevent nullreferences from being thrown (and instead returns
null). In other words, your
if evaluation will be false if either
The second example is closer to what you want, but it's not leveraging the best syntax. It can be cleaned up further, without introducing nesting:
if(eventArgs is OnZoomEventArgs zoomEventArgs && zoomEventArgs.focus != null)
eventArgs is OnZoomEventArgs zoomEventArgs will cast the object when it can (and return true), and will pass false if it cannot (thus directing you to the
else). Notice that you are able to use this casted object immediately in the same evaluation (
zoomEventArgs.focus != null).
I also removed the
_ suffix as it does not conform to any convention I've ever heard of.
My question is should I instead check for null reference this many times?
Should you check for nulls? Of course, it's part of the validation logic. But you should avoid writing individual failure clauses when the failure logic remains the same in all cases.
should I instead try to not pass a null reference
Never return null unless you intend to deal with it yourself.
When you intentionally throw a null, you are required to handle the potential nulls being returned. This is the honeytrap of developers. It's very easy to "just return null" and not write any proper handling, but it simply shifts the problem towards the caller, who now has to deal with nulls.
Dealing with nulls, as trivial as it may seem, requires the caller to know if and when nulls are being returned, and it burdens them with the task of dealing with it.
Avoiding wanton null returning as a good practice is further confirmed by the coming update of C#8, where nullable reference types are added specifically to address this "just return null" behavior. The name is a bit confusing, what it actually does is that it makes all reference types non-nullable, unless you explictly allow them to be nullable.
This is analogous to how you today can already choose to use
int?, for example. This behavior is simply going to be extended to reference types.
I tried to replace [...] with ternaries but I cannot as both SetLine() and ResetLine() have void as return type.
Don't abuse ternaries as "shorter ifs". If you want to shorten the line count of your
if, simply remove the braces (I'm not opposed to that, but this may clash with some style guides who enforce the use of brackets).
Ternaries should only be used in cases where you are assigning a value and would end up with a needlessly bulky
if block for what amounts to a simple assigning of a value.
Note that ternaries do not improve code readability on non-trivially readable code.
isActive ? "active" : "inactive" is readable because it is simple (and using an
if structure here would be needlessly bulky.
(eventArgs is OnZoomEventArgs zoomEventArgs && zoomEventArgs.focus != null) ? "passed" : "failed" is considerably less readable because it's no longer a trivially readable piece of logic.