11
\$\begingroup\$

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;
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Can differenceInDays be negative? Can sosire.TimeOfDay.Hours be greater than 23? \$\endgroup\$ Apr 5, 2013 at 13:00
  • \$\begingroup\$ No and No. It calculates de days off for a delegation. sosire = arrival, jumatateDeZiLbera = Half a day off, oZiLibera = one day off. \$\endgroup\$
    – nistrar
    Apr 5, 2013 at 13:03
  • \$\begingroup\$ The code returns only for last else's, other cases just modify the (member? local?) variable days. Please describe whether days is a local variable, and what is returned in case of e.g. tipDelegatie==true and differenceInDays > 2. I can assume that at the and days value is a result of this method, but it is not reflected in your code \$\endgroup\$
    – almaz
    Apr 5, 2013 at 13:08
  • \$\begingroup\$ Is it a bug that when differenceInDays == 2 && sosire.TimeOfDay.Hours == 22 you hit the final else? \$\endgroup\$ Apr 5, 2013 at 13:11
  • \$\begingroup\$ I modified it and changed the variables in english. Now? \$\endgroup\$
    – nistrar
    Apr 5, 2013 at 13:15

5 Answers 5

18
\$\begingroup\$

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;
\$\endgroup\$
1
  • 7
    \$\begingroup\$ Excellent. The only thing I would change would be to give meaningful names to the constants 2, 29, and 22. \$\endgroup\$ Apr 5, 2013 at 14:23
6
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • \$\begingroup\$ Relying on the zero value of the enum is somewhat brittle. Wouldn't you consider actually initialising howMany when you declare it more readable than a long comment explaining why you don't need to bother? \$\endgroup\$ Apr 12, 2013 at 9:08
  • \$\begingroup\$ Brittle? hmm.. It makes sense that the initial value of DaysOff would be "none". An int defaults to zero - same difference. The default value is what it is. The main point is defining/declaring "undefined"/"none" up front, the enum default just makes that particular value convenient to be defined as such. This is especially good when I'm converting text-based "code" values to enums. Helps trapping errors and keeps the enum-handling code from having to know about the "raw" code values. Also good when business rules do not specify a default value. \$\endgroup\$
    – radarbob
    Apr 16, 2013 at 2:42
1
\$\begingroup\$

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;
        }
    }
\$\endgroup\$
4
  • \$\begingroup\$ also string returns are usually a sign of ui and business together. have the ui transform the enum into a string (take for example the requirement to support multiple languages) \$\endgroup\$ Apr 5, 2013 at 14:37
  • 4
    \$\begingroup\$ I agree about strings, but I think your solution is over-architected. What you have done is produced 50 lines of highly structured code from 8 simple lines of business logic. Yes, your code is well-structured. But 8 lines are much easier to understand than 50 lines. It might be worth doing what you did if (when) business logic gets more complex, but as for current implementation that's too much (imho). \$\endgroup\$
    – almaz
    Apr 5, 2013 at 15:17
  • 3
    \$\begingroup\$ You can't seriously think this is easier to understand than the original code, can you? \$\endgroup\$
    – svick
    Apr 5, 2013 at 17:20
  • \$\begingroup\$ @almaz good point with the over architecture. have removed it from the original post. Still I get worried when business is inlined to get shorter code. You may have smaller code but it takes a while to see all the business requirements being implemented, in this case its probably the law business and getting it wrong could be expensive. \$\endgroup\$ Apr 7, 2013 at 13:50
0
\$\begingroup\$

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;
\$\endgroup\$
2
  • 4
    \$\begingroup\$ I disagree with using comments in this fashion. Your code should written in a way it conveys what it is accomplishing. Its easy to skip updating comments as the code changes, but the code will always be the source of truth. \$\endgroup\$ Apr 5, 2013 at 14:28
  • 1
    \$\begingroup\$ Absent more self-descriptive code, comments such as these are entirely desireable. \$\endgroup\$
    – radarbob
    Apr 5, 2013 at 17:32
0
\$\begingroup\$

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;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.