Validation in BLL, many ifs – Suggestions how to refactor what I think looks smelly

Posted on

Problem

We have an MVC application, which performs actions on a referral. We are added the business logic, to ensure that we can’t perform invalid actions on the referral due to it not being in the correct state. We have something that works, but it looks pretty smelly, however we are unsure how to improve it.

I feel like there has to be a better way. Any ideas how to improve the mass of if checks in the CanPerformEvent method? Thanks.

public abstract class ReferralTask : IReferralTask
{
    public async Task<bool> RunAsync()
    {
        Validate();
        var success = await RunTaskAsync();
        await LogEventAsync(success);
        return success;
    }

    protected void Validate()
    {
        if (!Referral.CanPerformEvent(Event))
        {
            throw new InvalidReferralStateException(Referral.Id, Event, Referral.ValidNextEvents());
        }
        OnValidate();
    }
}

public abstract class Referral
{
    public bool CanPerformEvent(ReferralEventEnum referralEvent)
        {
            var validNextEvents = ValidNextEvents();
            return validNextEvents.Contains(referralEvent);
        }

        public ReferralEventEnum[] ValidNextEvents()
        {
            // Get all the successful events for the referral.
            var successfulEvents = ReferralHistory
                .Where(h => h.Successful)
                .Select(h => h.EventId)
                .ToList();

            // Initialise list for valid next events.
            var validNextEvents = new List<ReferralEventEnum>();

            // If the request has been recieved by the portal...
            if (successfulEvents.Contains(ReferralEventEnum.RequestReceived))
            {
                // ...but not completed, it can be completed.
                if (!successfulEvents.Contains(ReferralEventEnum.RequestCompleted))
                {
                    validNextEvents.Add(ReferralEventEnum.RequestCompleted);
                }

                // ...but not notified, a notification can be sent.
                if (!successfulEvents.Contains(ReferralEventEnum.RequestNotificationSent))
                {
                    validNextEvents.Add(ReferralEventEnum.RequestNotificationSent);
                }
            }

            // If request completed and not accepted or refused, it can be accepted or refused.
            if (successfulEvents.Contains(ReferralEventEnum.RequestCompleted) && 
                !successfulEvents.Contains(ReferralEventEnum.ReferralAcceptedInPortal) &&
                !successfulEvents.Contains(ReferralEventEnum.ReferralRefusedInPortal))
            {
                validNextEvents.Add(ReferralEventEnum.ReferralAcceptedInPortal);
                validNextEvents.Add(ReferralEventEnum.ReferralRefusedInPortal);
            }

            // If the referral has been accepted in the portal...
            if (successfulEvents.Contains(ReferralEventEnum.ReferralAcceptedInPortal))
            {
                // ...but the acceptance request hasn't been sent, it can be sent.
                if (!successfulEvents.Contains(ReferralEventEnum.ReferralAcceptedRequestSent))
                {
                    validNextEvents.Add(ReferralEventEnum.ReferralAcceptedRequestSent);
                }

                // ...but provider update form not created, it can be created.
                if (!successfulEvents.Contains(ReferralEventEnum.ReferralAcceptedFormCreated))
                {
                    validNextEvents.Add(ReferralEventEnum.ReferralAcceptedFormCreated);
                }

                // ...and not updated or failed, it can be updated, delayed and failed.
                if (!successfulEvents.Contains(ReferralEventEnum.ReferralUpdatedInPortal) &&
                    !successfulEvents.Contains(ReferralEventEnum.ReferralFailedInPortal))
                {
                    validNextEvents.Add(ReferralEventEnum.ReferralUpdatedInPortal);
                    validNextEvents.Add(ReferralEventEnum.ReferralFailedInPortal);

                    // If all delays have been successfully completed, can delay again.
                    if (successfulEvents.Count(x => x == ReferralEventEnum.ReferralDelayInPortal) ==
                        successfulEvents.Count(x => x == ReferralEventEnum.ReferralDelayRequestSent))
                    {
                        validNextEvents.Add(ReferralEventEnum.ReferralDelayInPortal);
                    }
                }
            }

            // If refused in portal but request not sent, can send request.
            if (successfulEvents.Contains(ReferralEventEnum.ReferralRefusedInPortal) &&
                !successfulEvents.Contains(ReferralEventEnum.ReferralRefused))
            {
                validNextEvents.Add(ReferralEventEnum.ReferralRefused);
            }

            // If refusal request sent, but the referral hasn't been removed, it can be removed.
            if (successfulEvents.Contains(ReferralEventEnum.ReferralRefused) &&
                !successfulEvents.Contains(ReferralEventEnum.RemovedFromPortal))
            {
                validNextEvents.Add(ReferralEventEnum.RemovedFromPortal);
            }

            // If the referral has had a failed service start in the portal
            if (successfulEvents.Contains(ReferralEventEnum.ReferralFailedInPortal))
            {
                // ...but the form hasn't been updated, it can be updated.
                if (!successfulEvents.Contains(ReferralEventEnum.FailedServiceStartFormUpdated))
                {
                    validNextEvents.Add(ReferralEventEnum.FailedServiceStartFormUpdated);
                }

                // ...but failed service start request not sent, it can be sent.
                if (!successfulEvents.Contains(ReferralEventEnum.FailedServiceStartRequestSent))
                {
                    validNextEvents.Add(ReferralEventEnum.FailedServiceStartRequestSent);
                }
            }

            // If the failed service start request has been sent and form is created, but the referral hasn't been removed, it can be removed.
            if (successfulEvents.Contains(ReferralEventEnum.FailedServiceStartRequestSent) &&
                successfulEvents.Contains(ReferralEventEnum.FailedServiceStartFormUpdated) &&
                !successfulEvents.Contains(ReferralEventEnum.RemovedFromPortal))
            {
                validNextEvents.Add(ReferralEventEnum.RemovedFromPortal);
            }

            // If not every delay in portal has had its request sent, can send the delay in portal request.
            if (successfulEvents.Count(x => x == ReferralEventEnum.ReferralDelayInPortal) >
                successfulEvents.Count(x => x == ReferralEventEnum.ReferralDelayRequestSent))
            {
                validNextEvents.Add(ReferralEventEnum.ReferralDelayRequestSent);
            }

            // If the referral has been updated in the portal
            if (successfulEvents.Contains(ReferralEventEnum.ReferralUpdatedInPortal))
            {
                // ...but not sent request, can send request.
                if (!successfulEvents.Contains(ReferralEventEnum.UpdateCompletedRequestSent))
                {
                    validNextEvents.Add(ReferralEventEnum.UpdateCompletedRequestSent);
                }

                // ...but not created form, can create form.
                if (!successfulEvents.Contains(ReferralEventEnum.UpdateCompleted))
                {
                    validNextEvents.Add(ReferralEventEnum.UpdateCompleted);
                }

                // ...and not submitted actuals, can submit actuals.
                if (!successfulEvents.Contains(ReferralEventEnum.ProvideActualsInPortal))
                {
                    validNextEvents.Add(ReferralEventEnum.ProvideActualsInPortal);
                }
            }

            // If submitted actuals in portal and not created step, can create step.
            if (successfulEvents.Contains(ReferralEventEnum.ProvideActualsInPortal) &&
                !successfulEvents.Contains(ReferralEventEnum.ProviderActualsStepCreated))
            {
                validNextEvents.Add(ReferralEventEnum.ProviderActualsStepCreated);
            }

            // If actuals have been sent, the referral can be removed from the portal.
            if (successfulEvents.Contains(ReferralEventEnum.ProviderActualsStepCreated) &&
                !successfulEvents.Contains(ReferralEventEnum.RemovedFromPortal))
            {
                validNextEvents.Add(ReferralEventEnum.RemovedFromPortal);
            }

            return validNextEvents.ToArray();
        }   
}

Solution

Depending on you effort you want to put in your redesing, you coud use some kind of state pattern, and therefore just kow the valid events, instead of calculating them every time.

Or you just simply turn the logic arround. Instead of getting all valid events, and check if the requested is in the list, just check if the conditions for the requested event are met:

public bool CanPerformEvent(ReferralEventEnum referralEvent)
{            
    var successfulEvents = ReferralHistory
        .Where(h => h.Successful)
        .Select(h => h.EventId)
        .ToList();

    switch(referralEvent)
    {
        case ReferralEventEnum.RequestCompleted:
            return successfulEvents.Contains(ReferralEventEnum.RequestCompleted) && successfulEvents.Contains(ReferralEventEnum.RequestReceived)
        // a lot more cases
    }
}

EDIT: I just noticed, that you have a list of enums, and check for enums in that list. It is way mor efficient to use Flags Enums (Explanation as always on Stackoverflow)

Switch inside foreach

All the if‘s except one start with different Contains, so it would better be handled like this:

foreach(var element in successfulEvents)
{
    switch(element)
    {
        case ReferralEventEnum.RequestReceived:
            if (!successfulEvents.Contains(ReferralEventEnum.RequestCompleted))
                validNextEvents.Add(ReferralEventEnum.RequestCompleted);
            //...
            continue;
        case ReferralEventEnum.RequestCompleted:
            if (!successfulEvents.Contains(ReferralEventEnum.ReferralAcceptedInPortal) &&
                !successfulEvents.Contains(ReferralEventEnum.ReferralRefusedInPortal))
            {
                validNextEvents.Add(ReferralEventEnum.ReferralAcceptedInPortal);
                validNextEvents.Add(ReferralEventEnum.ReferralRefusedInPortal);
            }
            continue;

        // If not every delay in portal has had its request sent, can send the delay in portal request.
        case ReferralEventEnum.ReferralDelayInPortal:
            if(!successfulEvents.Contains(ReferralDelayRequestSent))
                validNextEvents.Add(ReferralEventEnum.ReferralDelayRequestSent);
            continue;
    }
}

The third case handles the only Count I have seen and it appears the ...Count > ...Count serves same purpose – to check that one flag is set and another is not. Can there be multiple flags of same value in the source list? That appears to be unlikely. At least adding multiple flags to returned array appears not to be desired (as it is checked to contain something).

The above solution is not only nicer, it is also faster (looping once instead of calling Contains in each if …it still contains some nested Contains but the first-level Contains are now in single loop).

Return IEnumerable<> instead of array

You don’t seem to need the array, you just need something that can answer Contains(ReferralEventEnum). Why constructing the list and converting it to array, when you can just yield return?

public IEnumerable<ReferralEventEnum> ValidNextEvents()
{
    //...

    foreach(var element in successfulEvents)
    {
        switch(element)
        {
            case ReferralEventEnum.RequestReceived:
                if (!successfulEvents.Contains(ReferralEventEnum.RequestCompleted))
                    yield return ReferralEventEnum.RequestCompleted;

Enum [Flags] or bit-field instead of List

If you do not have more than 64 events, then you can arrange the enum as flag-enum, each element having value of 2^i where i = 0..63 (RequestReceived = 1<<0, RequestCompleted = 1<<1, ...). Then the Contains becomes simple bit test: Contains(enum, flag) => (enum & flag) != 0;

It could be good idea to create your custom ReferralEventList, that would contain methods Contains and maybe GetEnumerator. It could use bit-operations (and, or, test to zero) at first, updated to some larger bit-field (list of bytes with bit-access) if you exceed 64 bits.

if not there add to next

This is very common pattern (if (!successfulEvents.Contains(...)) validNextEvents.Add(...)). Should be made a method. Either static (maybe extension) method or part of that custom ReferralEventList.

Leave a Reply

Your email address will not be published. Required fields are marked *