Complex if else

Question

How would you write this?

string halfADayOff = "Half a day off";
string oneDayOff = "One day off";
string days = "NoDaysOff";

if (typeOfDelegation)
    {

        if ((differenceInDays == 0) && (arrivalDay.TimeOfDay.Hours > 22))
            days = halfADayOff;
        else if ((differenceInDays == 1) && (arrivalDay.TimeOfDay.Hours > 22))
            days = halfADayOff;
        else if ((differenceInDays == 2) && (arrivalDay.TimeOfDay.Hours <= 22))
            days = halfADayOff;
        else if ((differenceInDays == 2) && (arrivalDay.TimeOfDay.Hours > 22))
            days = oneDayOff;
        else if (differenceInDays > 2)
            days = oneDayOff;
        else return days;
    }

    else
        if (differenceInDays > 29)
            days = oneDayOff;
        else
            return days;

   return days;

Answer 1

In order to simplify nesting I reordered conditions so that you can deal with simplest cases first, and return result as soon as you know it instead of capturing it in days variable. Also ternary operator looks like a good choice here...

const string noDaysOff = "NoDaysOff";
const string halfADayOff = "Half a day off";
const string oneDayOff = "One day off";
const int lateNightCutoff = 22; //TODO: couldn't come up with better name, update if you find one

if (!typeOfDelegation)
    return differenceInDays > 29 ? oneDayOff : noDaysOff;

if (differenceInDays > 2)
    return oneDayOff;

var arrivalHour = arrivalDay.TimeOfDay.Hours;

if (differenceInDays == 2)
    return arrivalHour > lateNightCutoff ? oneDayOff : halfADayOff;

return arrivalHour > lateNightCutoff ? halfADayOff : noDaysOff;

Answer 2

considerations

• Remove the else return days from both "halves" of the outer if. They are not needed.
• Use helpful names. What does differenceInDays mean?
• Initialize values near where they are used - a default value for days. That variable has class scope (I assume), so who knows what that value might if we hit the if-else default that didn't set it, but just returns it. We'd have to read LOTS of code to figure that out.
• Name boolean variables so they read well in the context of the if statement.
• Use enumerations instead of string literals when you have logical groupings.
• Use curly brackets in compound/complex if-else to avoid confusion and ambiguity.
• I don't understand what typeOfDelegation means, so I probably mis-re-named it below. That line essentially says "If something is a kind of delegation ...". How may types are there? If it either is or is not a "delegation" then "isDelegation" is better than "typeOfDelegation".
• I like what @almaz did in his answer to consolidate the logic, but too many returns makes code changes more error prone. In my opinion I'd re-structure the code if necessary to eliminate (most of) them. Yes, the code is short but that misses the point. I've seen short code like this re-written because multiple hard returns did not allow needed changes.
• encapsulate complex/long logic so the "higher level" code is clearer. The goal is not shorter code, but understanable and modular code.

considerations applied

public enum DaysOff {none, one, half};
protected int oneMonth = 29; 

    // let's assume the following was done earlier in code
    Timespan tripDuration = departureDay - arrivalDay;
    //end

    days = DaysOff.none;

    if(goneALongTime) {
        days = tripDuration.Days > oneMonth? DaysOff.one : days; 
    } else {
        days = GoneShortTime( tripDuration.Days );
    }

    return days;
    } // method end

    protected DaysOff GoneShortTime( int daysGone, arrivalDay ) {
        DaysOff howMany; // defaults to "none" (the underlying zero value of the enum)
        int endOfDay = 22;  // 10 pm 
        int arrivalHour = arrivalDay.TimeOfDay.Hours;

        if (( daysGone == 0 ) && ( arrivalHour > endOfDay ))
            howMany = DaysOff.half;
        else if (( daysGone == 1 ) && ( arrivalHour > endOfDay ))
            howMany = DaysOff.half;
        else if (( daysGone == 2 ) && ( arrivalHour <= endOfDay ))
            howMany = DaysOff.half;
        else if (( daysGone == 2 ) && ( arrivalHour > endOfDay ))
            howMany = DaysOff.one;
        else if ( daysGone > 2 )
            howMany = DaysOff.one;

        return howMany;
}

Answer 3

I wouldn't shorten it and i don't like comments (seperate discussion but it usualyl means your code is hard to read)

I would also try to see if you could avoid the daydifference and just pass in number of hours worked. It would be easier to maintain and probably correspond directly to the law you are obeying.

Anyway for example with the give restrictions and not 100% perfect but you get the point:

    public class GiveTheManABreak
    {
        public enum DaysOff
        {
            HalfADayOff,
            OneDayOff,
            NoDaysOff,
        }

        public DaysOff CalculatesDaysOff(bool typeOfDelegation, int differenceInDays, DateTime arrivalDay)
        {
            if (typeOfDelegation)
                return DelgateCalculateDaysOff(differenceInDays, arrivalDay);

            if (BeenHereTooLong(differenceInDays))
                return DaysOff.OneDayOff;

            if (ArrivedAfterTenAndHasBeenHereZeroOrOneDay(differenceInDays, arrivalDay.TimeOfDay.Hours))
                return DaysOff.HalfADayOff;

            if (Arrived10pmOrEarlierTwoDaysAgo(differenceInDays, arrivalDay.TimeOfDay.Hours))
                return DaysOff.HalfADayOff;

            if (ArrivedAfter10pmTwoDaysAgo(differenceInDays, arrivalDay.TimeOfDay.Hours))
                return DaysOff.OneDayOff;

            return DaysOff.NoDaysOff;
        }

        private bool BeenHereTooLong(int differenceInDays)
        {
            return differenceInDays > 2;
        }

        private bool ArrivedAfter10pmTwoDaysAgo(int differenceInDays, int arrivalTime)
        {
            if (differenceInDays != 2)
                return false;

            return arrivalTime > 22;
        }

        private bool Arrived10pmOrEarlierTwoDaysAgo(int differenceInDays, int arrivalTime)
        {
            if (differenceInDays != 2)
                return false;

            return arrivalTime <= 22;
        }

        private bool ArrivedAfterTenAndHasBeenHereZeroOrOneDay(int differenceInDays, int arrivalTime)
        {
            if (differenceInDays > 2)
                return false;

            return arrivalTime > 22;
        }

        private DaysOff DelgateCalculateDaysOff(int differenceInDays, DateTime arrivalDay)
        {
            return differenceInDays > 29 ? DaysOff.OneDayOff : DaysOff.NoDaysOff;
        }
    }

Answer 4

You could change the logic so it is smaller, but in its current form, it looks more readable and readability has a value. However, I think a few more comments might make it even more readable. Especially comments that show some examples.

//for example: from 10pm to midnight
if ((differenceInDays == 0) && (arrivalDay.TimeOfDay.Hours > 22))
    days = halfADayOff;
//from 10pm to 4am  or 10pm to 10am
else if ((differenceInDays == 1) && (arrivalDay.TimeOfDay.Hours > 22))
    days = halfADayOff;

Answer 5

The logic in the first case can be simplified by making two observations:

• differenceInDays == 0 behaves as differenceInDays == 1
• For d> 0, differenceInDays == d && arrivalDay.TimeOfDay.Hours > 22 behaves as differenceInDays == d+1 && arrivalDay.TimeOfDay.Hours <= 22

These observations give (following other answers in using an enum):

int halfDaysOff;
if (typeOfDelegation) {
    if (differenceInDays == 0) differenceInDays++;
    if (arrivalDay.TimeOfDay.Hours > 22) differenceInDays++;
    halfDaysOff = differenceInDays - 1;
}
else halfDaysOff = (differenceInDays > 29) ? 2 : 0;
switch (halfDaysOff) {
    case 0: return TimeOff.None;
    case 1: return TimeOff.HalfDay;
    default: return TimeOff.OneDay;
}