0
\$\begingroup\$

So this question might be a bit opinion based but I am a bit confused, say i have these two code snippets -

if (eventArgs != null && eventArgs is OnZoomEventArgs)
        {
            var zoomEventArgs = eventArgs as OnZoomEventArgs;
            if (zoomEventArgs.focus != null)
            {
                var line = zoomEventArgs.focus.GetComponent<AssemblyLine>();
                if (line != null)
                {
                    SetLine(line);
                } else
                {
                    ResetLine();
                }
            }
            else
            {
                ResetLine();
            }
        }
        else
        {
            ResetLine();
        }

and

var zoomEventArgs_ = eventArgs as OnZoomEventArgs;
        if(zoomEventArgs_.focus != null)
        {
            SetLine(zoomEventArgs_.focus.GetComponent<AssemblyLine>());
        } else
        {
            ResetLine();
        }

Which one is better in the sense of readability and in terms of performance maybe. My question is should I instead check for null reference this many times or should I instead try to not pass a null reference. Is there any other way I can make these code snippets more readable. I tried to replace

if (line != null)
                {
                    SetLine(line);
                } else
                {
                    ResetLine();
                }

with ternaries but I cannot as both SetLine() and ResetLine() have void as return type.

\$\endgroup\$
  • \$\begingroup\$ We need a description of the code and we also need to know that its purpose is. It's off-topic without this information and in its current form it is better suited for Software Engineering. \$\endgroup\$ – t3chb0t Feb 13 '19 at 11:18
  • \$\begingroup\$ I am sorry, i thought this site would suit, next time i will keep that in mind. \$\endgroup\$ – user192786 Feb 13 '19 at 12:03
1
\$\begingroup\$

The problem

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)

Note the ?. 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 zoomEventArgs_ or zoomEventArgs_.focus is null


The solution

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)
{
    SetLine(zoomEventArgs.focus.GetComponent<AssemblyLine>());
} else
{
    ResetLine();
}

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.


Your question

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 and 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.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer its really helpful. \$\endgroup\$ – user192786 Feb 13 '19 at 12:02