1

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.

4
  • I think the question both of you should be asking is "does either alternative approach confer benefits that are greater than the cost of refactoring?" Aug 24 '21 at 13:41
  • And both new approaches are less clear than the original code. Aug 24 '21 at 14:33
  • Instead of the try pattern, I prefer the Option type of the LanguageExt framework (see github.com/louthy/language-ext ) Aug 30 '21 at 13:23
  • 1
    It bothers me that no one has pointed this out yet: the response != null part is unnecessary. In C#, the is pattern does not match to null. In other words, GetResponse() is RestResponse response will only return true if response is not null already.
    – JMekker
    Sep 1 '21 at 16:33
2

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?

2

GetResponse() always returns a RestResponse object, so there is no need for pattern matching in this case; that part of the if statement always returns true by definition. It makes the code more complicated with no benefit.

Good code or bad code is not measured in the number of lines of code, but rather the content of those lines. Introducing a needless statement to save a LOC is not helpful.

-1

C# 8 introduced a feature called Nullable reference types. When you enable this feature (globally or in a specific context, see the docs) the compiler will give warnings when a variable is set to an expression that may be null. When you fix that warning, by passing in a default value or throwing an exception, or whatever is appropriate for the given situation, you can be sure that your method will always return a non-null object.

Therefor the clients of the method will not have to do any null checks!

In fact, if you do a null check, your IDE will probably give you a hint that the check is not required, because the variable can't be null.

Sometimes you may want to work with nullable reference types, and you still can. You can use question mark for that, for example string? foo; When using the foo variable, the compiler will warn you that it can be null and you should do a null check. But now it is explicit, you know exactly when a variable can be null or not.

So if you want to improve readability and at the same time improve the overall quality of your code, enable nullable reference types.

1
  • This line of thinking is not wrong, but when it comes to real world legacy code, enabling c#’s nullable reference types feature usually ends up in code that does not compile any more and requires a pretty huge rewrite.
    – Doc Brown
    Aug 24 '21 at 14:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.