Calculate attendance compliance based on attendance record

Question

This is part of a larger working system which I'm now trying to clean up. In the interests of making the system more test-able I'm splitting up the functions which can be split, but this 100-line function is messing me up.

Short overview, this takes 5 inputs:-

• matchedslots - A list of 4 times (nearest-match actual reporting schedule to the expected schedule in realslots)
• realslots - A list of 4 times (expected reporting schedule)
• leave - Fraction of the day that employee is on leave (0.2 is a 2 hour break, 0.5 is half-day leave, 1 is full-day leave)
• daytype - used elsewhere, but in this context 3 and above indicates a non-working day
• paid_lunch_period - special case for a certain class of employees who get paid for the lunch hour

And produces 6 outputs:-

• absent - 1, 0.5, or 0 indicating full/half/no absence on this day
• working - total number of hours worked
• shortearly - number of minutes the employee left early (for lunch or at day's end)
• shortlate - number of minutes the employee arrived late (morning or after lunch)
• overtime - number of minutes worked past closing time
• absentminutes - number of minutes the employee missed today (total)

The sheer number of if/else statements (and function length) convince me there must be a much better and more test-able way to write this, but I'm stumped.

def _calc_hours(matchedslots, realslots, leave, daytype, paid_lunch_period=False):
    '''
    Based on the matched slots, calculate how many days absent, how many
    minutes spent working (non-OT), how many minutes short, and how many
    minutes overtime done (pass through _rounding_OT first)

    Special case needs to be made for guard, where paid_lunch_period is
    set to True. In that case he gets an additional 1 working hour per
    day (assuming he was present).
    '''
    absent, working, shortearly, shortlate, overtime = 0, 0, 0, 0, 0
    extratime = 0  # Used to track possible extra time for non-working days
                   # This time is only present around the lunch period
    if leave not in (0, 0.2, 0.5, 1):
        raise(ValueError('Leave can only be 0, 0.2, 0.5, or 1'))
    # Variables to be used for calculating working time, may be overridden
    # later if staff late/early
    (clockin, lunchout, lunchin, clockout) = realslots
    if lunchout:
        morningminutes = _time_subtract(lunchout, clockin)
        eveningminutes = _time_subtract(clockout, lunchin)
        lunchduration = _time_subtract(lunchin, lunchout)
    else:  # Saturdays don't have lunch slots for some staff
        morningminutes = _time_subtract(clockout, clockin)
        eveningminutes = 0
        lunchduration = 0
    if matchedslots[0]:  # Signed in in the morning
        # Check for being late
        late = _time_subtract(matchedslots[0], realslots[0])
        if late > fuzzygap:  # fuzzygap leeway given for morning
            shortlate += late
            clockin = matchedslots[0]
        # Check if there IS a lunch time slot, and whether signout occured
        if realslots[1] and matchedslots[1]:
            # Check for possible short
            early = _time_subtract(realslots[1], matchedslots[1])
            if early > 0:
                shortearly += early
                lunchout = matchedslots[1]
            else:
                early = early + lunchduration
                extratime += -early if early < 0 else 0
    else:  # No morning sign-in, at least half-day absent
        absent += 0.5
        clockin = None
    if matchedslots[3]:  # Signed out sometime in evening/at night
        # Check if there IS a lunch time slot, and whether signin occured
        if realslots[2] and matchedslots[2]:
            # Check for possible short
            late = _time_subtract(matchedslots[2], realslots[2])
            if late > 0:
                shortlate += late
                lunchin = matchedslots[2]
            else:
                late = late + lunchduration
                extratime += -late if late < 0 else 0
        # Check for early signout
        early = _time_subtract(realslots[3], matchedslots[3])
        if early > 0:
            shortearly += early
            clockout = matchedslots[3]
        else:  # Did not leave early, check for overtime
            overtime = -early
    else:  # No evening/night sign-out, at least half-day absent
        absent += 0.5
        clockout = None
    # Calculate working hours
    if lunchout and lunchin:  # Not none meaning a normal day
        if clockin:
            working += _time_subtract(lunchout, clockin)
        if clockout:
            working += _time_subtract(clockout, lunchin)
            if ((_time_subtract(clockout, time(15,15)) > 0) and
                    (_time_subtract(time(15,0), lunchin) > 0) and
                    not paid_lunch_period):
                working -= 15  # tea break period
    else:  # A day without lunch hours
        if clockin and clockout:
            working += _time_subtract(clockout, clockin)
            # If there's overtime, subtract up to an hour because lunch hour
            # is not taken into account for this case yet
            overtime = overtime - 60 if overtime >= 60 else 0
    # If leave applied, this counts against absent
    absent = absent - leave if absent >= leave else 0
    if paid_lunch_period and absent == 0:
        if lunchout and lunchin:
            working += _time_subtract(lunchin, lunchout)
    if daytype > 2:  # Not a working day
        # Add in the extra time
        working += extratime
        # Hours worked are counted as overtime
        overtime += working
        working = 0
        # Can't be absent or early/late on non-working days
        absent, shortearly, shortlate = 0, 0, 0
    # Round out overtime hours
    overtime = _rounding_OT(overtime)
    # Calculate number of minutes absent
    absentminutes = 0
    if absent == 1:  # Full day absence
        absentminutes = morningminutes + eveningminutes
    elif absent == 0.5:  # Half day absence
        if matchedslots[0]:  # Signed in in morning, absence is evening
            absentminutes = morningminutes
    if leave == 0.2:  # Time Off (max 2 hours)
        if shortearly > shortlate:
            shortearly = 0 if shortearly < 120 else shortearly - 120
        else:
            shortlate = 0 if shortlate < 120 else shortlate - 120

    return absent, working, shortearly, shortlate, overtime, absentminutes

Basically what's being done should be clear from the comments. If statements check for a combination of cases (some staff forget to sign in/out, and management doesn't penalise that in the case of lunch-time hours; Saturdays don't have lunch time slots for staff whose shift is only till noon; half-day leave can still result in half-day absence).

Here's the test function I use to verify that it works (sorry, its a bit verbose). The first format of matchedslots and realslots can be seen to be a list of 4 datetime.time() instances. leave is one of the values (0, 0.2, 0.5, 1), daytype is 2 and below or 3 and above, paid_lunch_period is a boolean value.

from datetime import time

def test_calc_hours():
    daytype = {'work': 1, 'nonwork': 3}
    normalschedule = [time(8,30), time(12,30), time(13,30), time(18,0)]
    fridayadmin = [time(8,30), time(12,30), time(13,30), time(18,0)]
    fridaymuslim = [time(8,30), time(13,0), time(14,30), time(18,0)]
    fridaynon = [time(8,30), time(13,0), time(14,0), time(18,0)]
    saturdayadmin = [time(8,30), None, None, time(12,30)]
    saturdayothers = [time(8,30), time(12,30), time(13,30), time(15,0)]

    matchedslots = [time(8,31), time(12,31), time(13,6), time(18,16)]
    (absent, working, shortearly, shortlate, overtime, absentminutes
            ) = _calc_hours(matchedslots, normalschedule, 0,
                                        daytype['work'])
    assert (absent, working, shortearly, shortlate) == (0, 8*60 + 15, 0, 0)
    assert (overtime, absentminutes) == (0, 0)

    matchedslots = [time(9,45), time(12,31), time(13,6), time(18,16)]
    (absent, working, shortearly, shortlate, overtime, absentminutes
            ) = _calc_hours(matchedslots, normalschedule, 0.2,
                                        daytype['work'])
    assert (absent, working, shortearly, shortlate) == (0, 7*60, 0, 0)
    assert (overtime, absentminutes) == (0, 0)

    matchedslots = [time(8,31), time(12,31), time(12,46), time(16,1)]
    (absent, working, shortearly, shortlate, overtime, absentminutes
            ) = _calc_hours(matchedslots, normalschedule, 0,
                                        daytype['nonwork'])
    assert (absent, working, shortearly, shortlate) == (0, 0, 0, 0)
    assert (overtime, absentminutes) == (6*60+15, 0)

    matchedslots = [time(8,17), None, None, time(12,14)]
    (absent, working, shortearly, shortlate, overtime, absentminutes
            ) = _calc_hours(matchedslots, saturdayadmin, 0,
                                        daytype['work'])
    assert (absent, working) == (0, 224)
    assert (shortearly, shortlate) == (16, 0)
    assert (overtime, absentminutes) == (0, 0)

    matchedslots = [time(8,17), None, None, time(13,44)]
    (absent, working, shortearly, shortlate, overtime, absentminutes
            ) = _calc_hours(matchedslots, saturdayadmin, 0,
                                        daytype['work'])
    assert (absent, working) == (0, 240)
    assert (shortearly, shortlate) == (0, 0)
    assert (overtime, absentminutes) == (0, 0)

    matchedslots = [time(8,17), None, None, time(18,1)]
    (absent, working, shortearly, shortlate, overtime, absentminutes
            ) = _calc_hours(matchedslots, saturdayadmin, 0,
                                        daytype['nonwork'])
    assert (absent, working) == (0, 0)
    assert (shortearly, shortlate) == (0, 0)
    assert (overtime, absentminutes) == (8*60+30, 0)

    matchedslots = [time(8,39), None, None, time(17,0)]
    (absent, working, shortearly, shortlate, overtime, absentminutes
            ) = _calc_hours(matchedslots, saturdayothers, 0,
                                        daytype['work'])
    assert (absent, working) == (0, 5*60+21)
    assert (shortearly, shortlate) == (0, 9)
    assert (overtime, absentminutes) == (2*60+0, 0)

Answer 1

Too bad we can't run the tests, but of course it's good and absolutely necessary that they exist.

Some comments first while I read through the code:

• The docstring is okay, but for external reviews (moi) the arguments, constraints and return values aren't explained at all. Are there only male employees too?
• leave is compared with floating point numbers. I'd not use those for something that's essentially an enumeration. Not that the values make much sense for me either - 0.2 for two hours would mean that 0.5 is five hours, so a full day is ten hours? Instead I'd probably use an enumeration (with "small break", "half day", "full day" as values) or simply the number of hours allocated (as an integer of course). Not to mention that comparing floating point numbers with equality isn't something that you should really do. Since you actually do those comparisons later I'd even more strongly say don't do that.
• daytype should be an enum too then since an integer is super opaque. Let it have an is_work_day method or something.
• realslots and matchedslots are both unpacked and used directly - that seems fishy. It'd also make more sense to use a class, dictionary, or named tuple to give some names to the contained values.
• -early if early < 0 else 0 that can be written shorter as -min(early, 0) (although I'm not certain it's much clearer). Similarly max(shortearly - 120, 0) is shorter.
• There's this block with working = ...; overtime += working; working = .... That's less obvious than overtime += working + extratime; working = 0.
• The non-working day is being checked almost at the end of the function just to throw away everything computed before. That seems not very clear or efficient. Also, as the comment already states, absence doesn't apply to non-working days, so the block after it could again be skipped in this case.
• The "morning" and "afternoon" cases (matchedslots[0] and matchedslots[3]) are pretty similar - that'd be a very good block to reuse.
• Are lunchout and lunchin only set if it's a working day? Then the check for those two ("Calculate working hours") seems dubious and should explicitely check for the day "type". If not, maybe have a has_lunch on the working day enumeration instead/as well. Also, is there ever a case where only one of them is None? Because it certainly doesn't look like that would make sense with this code. Again it would make more sense to bundle these two dates together.

---

Now this is indeed a whole block of hard to follow code with way too many paths to take. Apart from I wrote above it'd be best to decompose this into smaller blocks (functions), approximately one for each "separate" computation. Of course that doesn't mean to simply split it up without thought, but to make it more understandable and more testable.

In a first iteration I feel that having more uniformity and better names (using enumrations, classes, ...) would make it easier to understand the logic instead of having track down the meaning of something[42].

It might also make sense to investigate whether using abstract durations instead of counting minutes and hours separately (and only doing the conversion at the end of the function) would make it more readable due to the use of regular arithmetic operators.