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

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?

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;
    }
}

StackExchange Code Review Q#59830, answer score: 6