# Calculate attendance compliance based on attendance record

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
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
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
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)


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

• 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.

• Thanks, a lot for me to digest. Agree my docstring is fairly horrible due to the high flux in my code. Unpacking of real/matchedslots is to 'save' the initial values (as they get modified halfway). Am sure there's a better way for that. non-working day checks should be moved up, only after you commented did I realise the wasted work done. lunchout/lunchin are only set on working days at present, but that may change in future. Currently no way for both to be None (these values are controlled for elsewhere in the system). – Ng Oon-Ee Jun 23 '16 at 8:44
• Anything I've not responded to I basically agree with and will update. – Ng Oon-Ee Jun 23 '16 at 8:45