Add advice on ternary operators and code coverage

[mvonballmo]

Every code-coverage tool I've seen tracks coverage by lines. A ternary operator or coalescing operator places multiple branches on a single line.

For example, the following code looks quite verbose.

if (expression != null)
{
  if (otherBaseExpression == null)
  {
    return false;
  }

  return expression.Equals(otherBaseExpression);
}

if (otherBaseExpression != null)
{
  return false;
}

return Equals(BaseValue, otherDynamicType.BaseValue);

While we could rewrite the code as follows, tightening things up a bit and not losing too much legibility, we would lose the line-by-line coverage we had before. That is, we won't be able to tell from our coverage whether each branch was really tested.

if (expression != null)
{
  return otherBaseExpression != null && expression.Equals(otherBaseExpression);
}

return otherBaseExpression == null && Equals(BaseValue, otherDynamicType.BaseValue);

[mvonballmo]

This applies not just for code coverage, but also for debugging and setting breakpoints.

It's not so easy to test the return value of Equals because the value constantExpression in the pattern-matching expression will show as null in the debugger until you've evaluated it. In this case, there isn't usually a way to execute only that expression (even in Rider), so you end up stepping over the whole line.

return obj is ConstantExpression constantExpression && Equals(ConstantValue, constantExpression.ConstantValue);

If you write the code with an if instead, as follows, then the code is easier to debug (and check for code coverage).

if (obj is ConstantExpression constantExpression)
{
  return Equals(ConstantValue, constantExpression.ConstantValue);
}

return false;

Similarly, the functional style that applies a mapping operation can also be difficult to debug. It's highly compact and legible, but it's very difficult to set a breakpoint. The Zip line below, applies the function Equals for the corresponding arguments in the two lists Nodes and Expression.Nodes and then returns a value indicating whether they are all true.

if (obj is IExpression otherExpression && GetType() == otherExpression.GetType())
{
  return Nodes.Zip(otherExpression.Nodes, Equals).All(o => o);
}

return false;

It is nearly impossible to break anywhere in this expression.

If you make it even more compact, then it's not only illegible, it's not debuggable and not coverable.

return obj is IExpression otherExpression && GetType() == otherExpression.GetType() && Nodes.Zip(otherExpression.Nodes, Equals).All(o => o);


A more debuggable version would be to unroll the operation so that you can set a breakpoint.

if (obj is IExpression otherExpression && GetType() == otherExpression.GetType())
{
  foreach (var (o1, o2) in Nodes.Zip(otherExpression.Nodes, (o1, o2) => (o1, o2)))
  {
    if (!Equals(o1, o2))
    {
      return false;
    }
  }

  return true;
}

return false;

It's not nearly as pretty and it allocates tuples for each item (instead of booleans), but it is more debuggable. Prefer the shorter (but not the shortest) form if there is strong need for debugging or coverage. If the need arises, you can always rewrite individual instances.