CODEWITHSUNDEEP
c#nullability
Urs Meili
Urs Meili

Reputation: 809

Is it considered bad practice to use a "string?" parameter?

In a system where every customer has a unique e-mail address, there is the following code to retrieve the customer id by it's e-mail:

public int? GetCustomerIdByEMail(string? email) {
   if(email==null) return null;
   return DB.Query<Customer>().Where(c=>c.EMail==email).Select(c=>c.Id).SingleOrDefault();
}

Is it considered bad practice to have the email parameter of type string? (instead of string), given that null is not considered a valid e-mail?

Having code like this spares the user of the code to write

var id = email!=null ? GetCustomerIdByEMail(email) : null

instead he can just write

var id = GetCustomerIdByEMail(email)

But personally I think it is a code smell to handle the null case like this, since null is not considered an e-mail address.

What do you people think about this problem?

Upvotes: 1

Views: 167

Answers (2)

quetzalcoatl
quetzalcoatl

Reputation: 33506

The purpose of functions/methods is to hide code away, and make the calling code easier to analyze by us, humans. For the computer, there'd be little difference whever the if-null check was inside or outside this function. (*)

Having that said, which calling code do you prefer to see?
For me, that's the shorter one, it's clear, uncluttered, mostly-understandable at first glance. Make it 2 or 5 such parameters, and difference in amount of IFs visible on screen is just huge (considering the calling code may be doing a series of various such get-foo-by-bar).
The fact that the result is int? not int is a 'gotcha' moment at first, but but that's something that comes with experience. You just have to spend some time to get used to any new coding style, that's all.

But calling code isn't the only code you read.

Let's now see the function itself.
Which version of the function's code, with if-null inside, or without it, would you prefer?
For me, that doesn't really matter. That single if is so trivial, it doesn't complicate that function at all. Were there 2 or 5 of them, still the same, since they'd be at the beginning, and would be repetitive, and that'd form an obvious parameter checking prologue. OTOH, if they were scattered across screen-sized function body, there'd be another thing, but as it is now, nope, no difference for me.

This and that together make me say: leave that if inside, especially if that function is called from many places (which would lead to copying that if-null to multiple places).

However, since you have doubts about this function and that parameter, I'd focus instead on why you have them. Maybe that's just that the name of GetCustomerIdByEMail suggests you something that, conceptually, doesn't particularly go well with a pass-through null behavior for you? How about changing the name to TryGetCustomerIdByEMail or MaybeGetCustomerIdByEMail(**)? Simple things like that can sometimes really make a difference.

(*) for nit-pickers, yes, yes, it MAY do a difference to the computer as well, branching, inlining, cache, and other, but I believe it's not important in this topic, since OP didn't state anything about workload size or performance.

(**) in this case, I really suggest Maybe. See this contrived example:

var id0 = MaybeGetCustomerIdByEMail(...);
var id1 = MaybeGetCustomerIdByFirstLastName(...);
var id2 = MaybeGetCustomerIdByInsuranceNumber(...);
var id3 = MaybeGetCustomerIdByAgreementNumber(...);

var ids = new[id0,id1,id2,id3].Where(id => id!=null).Distinct();
if(ids.Empty()) // throw customer not found
if(ids.Skip(1).Any()) // throw customer ambigous

var customerId = ids.Single();

By no means this is a perfect piece of code. Just a sketch. And of course your case may be vastly different. But I hope that shows why I'm expressing this, probably unpopular, view.

Upvotes: 1

Agent_L
Agent_L

Reputation: 5411

Nullable isn't a code smell per se.

But personally I think it is a code smell to handle the null case like this, since null is not considered an e-mail address.

null doesn't represent wrong value, null represents lack of one. If a lack of an email is a normal situation, then your solution is right.

However, the real question is "is lack of an email a normal situation" and most likely the answer is "no". Don't engineer your methods to handle situations they can't handle. Make them require the email. Handle null as an exception one level up. Handling exceptional situations is literally the purpose of exceptions. And the whole point of explicit nullablilty is to get this exception check for free.

The code smell is going against "fail-fast" rule: hiding an error and continuing a code path that has no hope of succeeding.

Upvotes: 2

Related Questions