C# – Logical Evaluation with Pattern Matching

I'm working with another developer and we're trying to clean up and modernize some legacy code; all the while, trying to keep things readable and remain somewhat surgical with changes _[1]. There's a method called GetResponse that returns a RestResponse object _[2]:

private RestResponse GetResponse();

Sprinkled throughout a static helper class (we're moving towards OOP, so let's not focus on this point _[3]), we have calls to this method that, get the object, determine if it's null, and, if it's not, does some processing:

RestResponse response = GetResponse();
if (response != null) {
    
}

With this in mind, the developer I'm working with, pointed out that the above snippet can be simplified to the following by using pattern matching:

if (GetResponse() is RestResponse response && response != null) {
    
}

However, my thought on the matter is simply, should it? For starters, I believe this is an abusive use of pattern matching since it is known that the return type of GetResponse is a RestResponse. Additionally, I believe it adds complexity to the logical evaluation, which forces other developers to slow down when reviewing or maintaining the code. All of that negativity and more, to save a single line of code? As such, I'm not exactly for the idea, but being that our environment is collaborative, I'm trying to keep an open mind. As such, I'd like to weigh in other opinions from the community before I solidify my opinion.

Personally, I believe the try pattern (e.g. if (TryGetResponse(out RestResponse response))) would be a better fit for the situation, but that's for another time.

Which is easier to read, understand and maintain; and, why?

Footnotes

_{1: By somewhat surgical, I mean that we're not trying to refactor chunks into new methods, objects, etc. Method bodies should still do the job they're intended to do, and everything they did before, they should still do. This is for the benefit of source control.}

_{2: The only portions of the GetResponse method that are relevant to my question are the method name and the return type.}

_{3: I have an additional task later to move things to an OOP approach and break it out of large monolithic methods; but, one step at a time.}

Best Answer

To me, both variants look almost equally readable. Note the second one is almost identical in length compared to the first: it contains only one keyword more (is), the only real difference is the order in which the keywords are arranged. Note the response variable has also the same scope (!) in both variants.

Hence, this mostly boils down to be a matter of taste.

Of course, when additional conditions come in, breaking them down into a few separate steps is probably more readable, and for the first one this might be a little bit simpler: I would prefer

  RestResponse response = GetResponse();
  if (response != null && response.IsValid()) {

  }

over

  if (GetResponse() is RestResponse response && response != null && response.IsValid()) {

  }

but there are surely people who would suggest to break the second conditional into multiple lines like this

  if (GetResponse() is RestResponse response && 
      response != null && 
      response.IsValid()) {
  }

and say this is equally readable.

So I would leave such code currently in the form as it is written - regardless if it is in the first or second variant. Changing it always has a small risk of introducing typos, and when there is actually no benefit, why take that risk?

Related Solutions

SOLID OOP Design – OOP Design Pattern Representing All SOLID Principles

One of the most popular design pattern among the community here is the strategy pattern. And yes, if you build some example code around this pattern, you can demonstrate all the SOLID principles:

S = is fulfilled when each strategy subclass is only responsible for exactly one task, and the "context" class does not take responsibilities which belong into the strategy classes
O = you can add new strategies afterwards ("open for extentions") without the need to change the internals of the context ("closed for modifications")
L = this means to implement the strategy subclasses correctly with the semantics defined by the strategy interface, not changing the semantics in any of the subclasses. LSP would not be fulfilled when a subclass implements the strategy interface in a manner which breaks the parts of the context using the strategy.
I = is fulfilled when the strategy base class offers only a small, single-purpose interface (like the one "execute" method)
D = the context relies completely on the abstract strategy interface, it gets the concrete strategy "injected" from outside (for example, at construction time) and does not make any assumptions about, for example, a fixed set of subclasses, or the availability of specific subclasses.

Note that the pattern itself does not guarantee your code to be SOLID, it is more that the SOLID principles help you to implement the pattern correctly. You may consider to show your students not only correct examples for applying the SOLID principles, but also counter-examples in context of a strategy pattern, showing code which breaks each of the five principles. Or even better: make this an exercise for them.

C# – Most Elegant Way to Write a Try Method in C# 7

Use an Option type, which is to say an object of a type that has two versions, typically called either "Some" (when a value is present) or "None" (when there isn't a value) ... Or occasionally they're called Just and Nothing. There are then functions in these types that let you access the value if it is present, test for presence, and most importantly a method that lets you apply a function that returns a further Option to the value if it's present (typically in C# this should be called FlatMap although in other languages it is often called Bind instead... The name is critical in C# because having s method of this name and type let's you use your Option objects in LINQ statements).

Additional features may include methods like IfPresent and IfNotPresent to invoke actions in the relevant conditions, and OrElse (which substitutes a default value when no value is present but is a no op otherwise), and so on.

Your example might then look something like:

myMessageQueue.TryDeque()
    .IfPresent( dequeued => MyMessagingClass.SendMessage(dequeued))
    .IfNotPresent (() =>  Console.WriteLine("No messages!")

This is the Option (or Maybe) monad pattern, and it is extremely useful. There are existing implementations (eg https://github.com/nlkl/Optional/blob/master/README.md) but it isn't hard to to your own either.

(You may wish to extend this pattern so that you return a description of the error cause instead of nothing when the method fails ... This is entirely achievable and is often called the Either monad; as the name implies you can use the same FlatMap pattern to make it easy to work with in that case, too)

Footnotes

Best Answer

Related Solutions

SOLID OOP Design – OOP Design Pattern Representing All SOLID Principles

C# – Most Elegant Way to Write a Try Method in C# 7

Related Topic