Too many null checks in an XML-to-XML transformation

Question

I'm writing a conversion mechanism in C#. I deserialize a big XML document of one type and return a big XML document of another type (via serialization of another object).

While I'm doing the conversion, I find myself doing a TON of null checks (checking whether or not a certain property has a value, which then would need to be translated). Here's an example piece of code:

if (MyPatientCareReport.eHistory != null && // Check if eHistory field exists
 MyPatientCareReport.eHistory.eHistoryCurrentMedsGroup != null && // Check if group exists
 MyPatientCareReport.eHistory.eHistoryCurrentMedsGroup.Count > 0 && // Make sure items exist
 MyPatientCareReport.eHistory.eHistoryCurrentMedsGroup[0].eHistory12 != null && // Null check
 MyPatientCareReport.eHistory.eHistoryCurrentMedsGroup[0].eHistory12.PN != "8801015") // None reported value (NEMSIS)
{
 string PertinantNegatives = MyPatientCareReport.eHistory.eHistoryCurrentMedsGroup[0].eHistory12.PN;
 if (PertinantNegatives == "8801019" || PertinantNegatives == "8801023")
 {
 // Add a new XML serializable boolean to the CDA object
 BL MyBL = new BL();
 MyBL.nullFlavor = "NI";
 MyCurrentlyOnMedicationObservation.value.Add(MyBL);
 }
 else
 {
 MyCurrentlyOnMedicationObservation.value.Add(new BL(true));
 }
}
else
{
 MyCurrentlyOnMedicationObservation.value.Add(new BL(false));
}

To me this look ugly. Should I be doing this a different way? If so, how?

• dcastro: You'll be glad to know that C# 6 will address this with the Null Propagation ?. operator. You'll be able to do this: Console.WriteLine( MyPatientCareReport?.eHistory?.eHistoryCurrentMedsGroup?[0]?.eHistory12 ).
• Matt: @dcastro's comment is exactly what I was looking for... a way to access inner elements of an object without null-checking every step of the way. Now that I know this is not possible (yet), maybe we can find another way to do this.

Answer 1

Your problem isn't null checks, it's a lack of encapsulation. This code is rifling through the metaphorical pockets of myPatientCareReport, like an overly familiar paperboy [PDF].

Let's look at what it knows:

• myPatientCareReport has a field eHistory
• eHistory has a field eHistoryCurrentMedsGroup
• eHistoryCurrentMedsGroup is an array (or at least has an indexer)
• the order of elements in eHistoryCurrentMedsGroup is meaningful
• the elements of eHistoryCurrentMedsGroup have a field eHistory12
• eHistory12 has a field PN
• PN is a string with some opaque meaning (see also: stringly typed)

If any of these implementation details were to change, you would end up rewriting a lot of code.

In object-oriented programming, encapsulation is the term for not only grouping data and behavior, but also hiding data and behavior implementation details within a class ... so that the inner workings of a class are not exposed. This reduces the chances that callers will modify the data inappropriately or program according to the implementation, only to have it change in the future. -- Essential C# 5.0

Perhaps it's time for a Demeter Transmogrifier?

Code that violates the Law of Demeter is a candidate for Hide Delegate, e.g. manager = john.getDepartment().getManager() can be refactored to manager = john.getManager(), where the Employee class gets a new getManager() method. However, not all such refactorings make as much sense. Consider, for example, someone who’s trying to kiss up to his boss: sendFlowers(john.getManager().getSpouse()). Applying Hide Delegate here would yield a getManagersSpouse() method in Employee. Yuck.

Now it's hard to suggest what this could look like because I'm not familiar with the problem domain, and a lot of the names here are meaningless to me (eHistory12? is there an eHistory13?). But you would probably want the code to end up looking something like this:

if (patientReport == null)
{
 ...
 return;
}
var currentMedicationGroup = patientReport.GetCurrentMedicationGroup(0);
currentOnMedicationObservation.Value.Add(GetBoolean(currentMedicationGroup));
private static BooleanValue GetBoolean(MedicationGroup medicationGroup)
{
 if (medicationGroup != null && medicationGroup.HasPertinentNegatives)
 {
 var pertinentNegatives = medicationGroup.GetPertinentNegatives();
 if (pertinentNegatives == PertinentNegatives.Refused ||
 pertinentNegatives == PertinentNegatives.UnableToComplete)
 {
 return new BooleanValue { NullFlavor = NullFlavor.NoInformation };
 }
 else
 {
 return BooleanValue.True;
 }
 }
 else
 {
 return BooleanValue.False;
 }
}

Answer 2

I would write an extension method to check whether the above condition holds true. and I hope eHistoryCurrentMedsGroup is a list of some object so you could use Any().

public static class HistoryValidatorExtension
{
 public static bool IsHistoryValid(this MyPatientCareReport myPatientCare)
 {
 return myPatientCare.eHistory != null && 
 myPatientCare.eHistory.eHistoryCurrentMedsGroup != null && 
 myPatientCare.eHistory.eHistoryCurrentMedsGroup.Any() && 
 myPatientCare.eHistory.eHistoryCurrentMedsGroup[0].eHistory12 != null;
 } 
}

OR declare a variable and put the condition there. so you can avoid creating extension method.

var historyValidCondition=MyPatientCareReport.eHistory != null &&
 MyPatientCareReport.eHistory.eHistoryCurrentMedsGroup != null && 
 MyPatientCareReport.eHistory.eHistoryCurrentMedsGroup.Any() && 
 MyPatientCareReport.eHistory.eHistoryCurrentMedsGroup[0].eHistory12 != null)

Answer 3

Your actual problem is that you end up doing this:

MyPatientCareReport.eHistory.eHistoryCurrentMedsGroup[0].eHistory12.PN

Not only it's not very object oriented, but such chaining can quickly become unreadable and difficult to maintain. Even if we suppose that your naming conventions are clear in your particular domain-specific case (and any other developer working on this project would know perfectly well what PN stands for or what 8801023 is), the:

A.eHistory.B[0].eHistory12.C

alone looks very strange: why is history inside a history? Why history 12? Why e? Why an array?

The array alone looks too weird. What is 0? A magic value? Doesn't your array hide a data structure, and if it does, why aren't you using a more clear representation of the data?

If you start moving the methods to where they belong, you'll end up removing those complicated chains and ambiguous types. Refactored code would finally look similar to:

public BL GenerateObservation(myPatientCareReport)
{
 if (!myPatientCareReport.History.ContainsNemsis())
 {
 var pertinantNegatives = myPatientCareReport.History.PertinentNegatives;
 if (pertinantNegatives.ContainNemsisDerivatives())
 {
 // Add a new XML serializable boolean to the CDA object
 var myBL = new BL(); // TODO: Replace "BL" by a readable name.
 myBL.nullFlavor = "NI";
 return MyBL;
 }
 else
 {
 return new BL(true);
 }
 }
 else
 {
 return new BL(false);
 }
}
...
var observation = this.GenerateObservation(myPatientCareReport);
myCurrentlyOnMedicationObservation.Add(observation);

Next refactoring introduces guard clauses:

public BL GenerateObservation(myPatientCareReport)
{
 if (myPatientCareReport.History.ContainsNemsis())
 {
 return new BL(false);
 }
 var pertinantNegatives = myPatientCareReport.History.PertinentNegatives;
 if (!pertinantNegatives.ContainNemsisDerivatives())
 {
 return new BL(true);
 }
 // Add a new XML serializable boolean to the CDA object
 var myBL = new BL(); // TODO: Replace "BL" by a readable name.
 myBL.nullFlavor = "NI";
 return MyBL;
}
...
var observation = this.GenerateObservation(myPatientCareReport);
myCurrentlyOnMedicationObservation.Add(observation);