Replacing conditional with something?

Question

I have the following nested if clause, I was wondering if I, by applying some pattern, can simplify it?

The code checks to see if it needs an authorizationStartDate, and if it does, but does not have it, returns true.

I've considered the Strategy Pattern, the "Replace conditional with polymorphism" method, the Specification Pattern and various others, but I haven't found anything which I liked.

private bool IsMissingAuthorizationStartDate(ApplicationStatusData data)
    {
        if (data.ApplicationStatus == ApplicationStatusType.ApplicationApproved)
        {
            if (data.ApplicationPurpose == ApplicationPurpose.New)
            {
                if (data.ProductStatus?.ProductStatusType == ProductStatusType.ApplicationForNewProductReceived)
                {
                    if (data.ApplicationTypePesticide == ApplicationTypePesticide.Authorisation ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.ProvisionalAuthorisation ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.MutualRecognition ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.Derogation ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.DispensationPreviousAssessment ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.ResearchAndDevelopmentPurposesExperimentOrTestProgram ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.ResearchAndDevelopmentPurposesExperimentOrTestProgramKnownProduct ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.ParallelTradePermit ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.Copy
                        )
                    {
                        if (!data.AuthorizationStartDate.HasValue)
                        {
                            return true;
                        }
                    }
                }
            }
            else if (data.ApplicationPurpose == ApplicationPurpose.Renewal)
            {
                if (data.ProductStatus.ProductStatusType == ProductStatusType.ProductAuthorised)
                {
                    if (data.ApplicationTypePesticide == ApplicationTypePesticide.ReAuthorisation ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.ParallelTradePermit ||
                        data.ApplicationTypePesticide == ApplicationTypePesticide.Copy
                        )
                    {
                        if (!data.AuthorizationStartDate.HasValue)
                        {
                            return true;
                        }
                    }
                }
            }
        }

        // else
        return false;
    }

Answer 1

The pattern I would use here is just encapsulation. The nesting here is hard to follow, and worsened by the equality comparisons. If possible, instead of exposing the raw field, try encapsulating the intent.

e.g. Instead of if (data.ApplicationPurpose == ApplicationPurpose.Renewal) try extending ApplicationStatusData with a property like

bool IsRenewalApplication 
{
 get 
 {
  return this.ApplicationPurpose == ApplicationPurpose.Renewal;
 }
}

so your code reads cleaner, with more expression: if (data.IsRenewalApplication) { ... }

Particularly where you have that massive if this or that or that or that, put it under a well-named property like IsInterestingPesticide.

If you can't change ApplicationStatusData for some reason, you can do the same thing with member functions that return Boolean values, expressing the same intent.

HTH!

PS, You might even want to encapsulate the entirety of the nested-ifs into a single concept. Then you'd just have 2 Boolean tests before you return false.

Answer 2

I suspect you might want to take a look at the next level up in the code, the fact its returning a boolean indicates this is being used in a conditional by something else.

That said I do usually like the chain of responsibility pattern for this sort of thing. But I personally wouldn't have it return a boolean, I'd have the responsible object perform an action if it was determined to be responsible for that type of data (i.e. Another level up).

Just an option for you to consider, there isn't a hard and fast rule for this kind of thing.

Answer 3

This doesn't really answer to your question which was about design pattern, but that might still interest you. You could rewrite your method like that.

First two arrays:

private ApplicationTypePesticide[] typePesticidesNewPurpose
    = new ApplicationTypePesticide[]
{
    ApplicationTypePesticide.Authorisation,
    ApplicationTypePesticide.ProvisionalAuthorisation,
    ApplicationTypePesticide.MutualRecognition,
    ApplicationTypePesticide.Derogation,
    ApplicationTypePesticide.DispensationPreviousAssessment,
    ApplicationTypePesticide.ResearchAndDevelopmentPurposesExperimentOrTestProgram,
    ApplicationTypePesticide.ResearchAndDevelopmentPurposesExperimentOrTestProgramKnownProduct,
    ApplicationTypePesticide.ParallelTradePermit,
    ApplicationTypePesticide.Copy
};

private ApplicationTypePesticide[] typePesticidesRenewalPurpose
    = new ApplicationTypePesticide[]
{
    ApplicationTypePesticide.ReAuthorisation,
    ApplicationTypePesticide.ParallelTradePermit,
    ApplicationTypePesticide.Copy
};

Then your previous method becomes:

private bool IsMissingAuthorizationStartDate(ApplicationStatusData data)
{
    return data.ApplicationStatus == ApplicationStatusType.ApplicationApproved
        && (IsMissingAuthorizationStartDatePart2(data, ApplicationPurpose.New,
                ProductStatusType.ApplicationForNewProductReceived, typePesticidesNewPurpose)
            || IsMissingAuthorizationStartDatePart2(data, ApplicationPurpose.Renewal,
                ProductStatusType.ProductAuthorised, typePesticidesRenewalPurpose));
}

private bool IsMissingAuthorizationStartDatePart2(ApplicationStatusData data,
    ApplicationPurpose purpose, ProductStatusType statusType,
    params ApplicationTypePesticide[] typePesticides)
{
    return (data.ApplicationPurpose == purpose
        && data.ProductStatus.ProductStatusType == statusType
        && statusType.Any(st => data.ApplicationTypePesticide == st)
        && !data.AuthorizationStartDate.HasValue);
}

Note: you can remove the params keyword if you always call the method like in this example.
You should also think about rename the part2 method.