Validation null values (preconditions)

Question

I would like to know an alternative, more elegant way to write the following methods.

I am especially not enthusiastic of the nested if statement or if for validate null values.

Now there are 3 if statement for null validation.

Any method:

 private void LoadAlertas()
 {
 // *** OMITTED ***
 // Nullable object must have a value
 var PortalUser = HttpContext.Current.User.PortalUser();
 if (string.IsNullOrWhiteSpace(PortalUser)) throw new ArgumentNullException("HttpContext.Current.User.PortalUser is null");
 var personalData = PersonalData.getPersonalData(PortalUser);
 if (personalData == null) throw new ArgumentNullException("PersonalData.getPersonalData is null para " + PortalUser);
 if (!personalData.iID_PersonalData.HasValue) throw new ArgumentNullException("personalData.iID_PersonalData is null");
 var userLogado = Bll.PortalUser.getUsarioPortalByIdPersonalData(personalData.iID_PersonalData.Value);

Another method:

 protected void Btn_Accion_Click(object sender, EventArgs e)
 {
 // *** OMITTED ***
 // Nullable object must have a value
 var PortalUser = Page.User.PortalUser();
 if (string.IsNullOrWhiteSpace(PortalUser)) throw new ArgumentNullException("Page.User.PortalUser is null");
 var personalData = MyPortal.AdminManager.Bll.PersonalData.getPersonalData(PortalUser);
 if (personalData == null) throw new ArgumentNullException("PersonalData.getPersonalData is null para " + PortalUser);
 if (!personalData.iID_PersonalData.HasValue) throw new ArgumentNullException("personalData.iID_PersonalData is null");
 var userLogado = MyPortal.AdminManager.Bll.PortalUser.getUsarioPortalByIdPersonalData(personalData.iID_PersonalData.Value);
 var userConsultado = setuserConsultado();

• IEatBagels: We lack context to help you. What is userLogado used for? Why would any of these variables be null? Where is the data taken from? A Web API, a database, an user input? Also, I think you're using ASP.Net, please add this tag to your question.

Answer 1

Why are you using ArgumentNullException? PortalUser isn't an argument, personalData isn't an argument, etc.

---

• PortalUser should be camelcase.
• getPersonalData should be PascalCase. Ditto getUsarioPortalByIdPersonalData. Ditto setuserConsultado (and "User" should be capitalized).
• iID_PersonalData should be PascalCase; moreover it seems to be using Hungarian notation which is against the C# standards, and it contains an underscore which is also against the C# standards.

---

I can't say much about the code, since there's so little of it. It doesn't look that good, to be honest, and I'd certainly move it to a specific method since both blocks seems to be part of a much longer method.

• Kiquenet: Legacy code. Which exception type can I use if not ArgumentNullException ?
• IEatBagels: @Kiquenet Well, what's the scenario? Is it "normal" that PortalUser is null? You could use KeyNotFoundException but without context that is hard to tell.

Answer 2

The problem you have at the moment is that you mix pulling the data and the rest of your code (that we don't see because you didn't include it).

Extract this code in a method :

private ??? getUserLogado(string portalUser)
{
 if (string.IsNullOrWhiteSpace(PortalUser)) 
 return null;
 var personalData = PersonalData.getPersonalData(PortalUser);
 if (personalData == null || !personalData.iID_PersonalData.HasValue) 
 return null; 
 return Bll.PortalUser.getUsarioPortalByIdPersonalData(personalData.iID_PersonalData.Value);
}

Then, if this method returns null, you can throw a custom exception. You need to figure why this didn't work, 'cause I don't know because of lack of context. Is it because of wrong credentials? A user that doesn't exist? Find that out, throw a custom exception. ArgumentNullException is the worst exception you can throw at the moment (Excluding Exception, of course). The argument isn't null (in your case, in my method it can be), the argument lead to wrong results. You should also wonder if it's normal that you throw an exception? Think about it, Outlook probably doesn't throw an exception each time I mess up my email/password. Maybe that a null userLogado implies that you should show a message to the user?

Answer 3

What about wrapping all this in a Try/Catch, assuming you'll get valid data, and throw a custom exception ? That way you will have your custom exception message and the real exception in the innerException.

I will do something like this

private void LoadAlertas()
 {
 try
 {
 // Nullable object must have a value
 var PortalUser = HttpContext.Current.User.PortalUser();
 var personalData = PersonalData.getPersonalData(PortalUser); 
 var userLogado = Bll.PortalUser.getUsarioPortalByIdPersonalData(personalData.iID_PersonalData.Value);
 }
 catch (Exception ex)
 {
 throw new NullReferenceException("Value needed", ex);
 }
 }

You can take a look at Best Practices for Exceptions

Use exception handling if the event doesn't occur very often, that is, if the event is truly exceptional and indicates an error (such as an unexpected end-of-file). When you use exception handling, less code is executed in normal conditions.

Use the programmatic method to check for errors if the event happens routinely and could be considered part of normal execution. When you check for errors programmatically, more code is executed if an exception occurs.

• IEatBagels: I really don't think a NullReferenceException should be thrown in your catch. NullReferenceException is an error that should never be thrown in a production code. It's bad and makes baby pandas cry! And, catching Exception is also a bad practice. If you were to stick to your code, you should catch the NullReferenceException as it's the only exception that we want to catch! :)