Simple greeting program based with timezone offset

Question

I made a simple set of functions that returns a greeting based on the current time, the user's timezone offset, and the user's level. For context I wrote this as a part of a larger chat bot application. I am not sure if there is a better way to handle the if/elif/else blocks in both functions.

Please let me know of places of improvement.

"""A module for creating greeting strings based on a user's timezone
offset and user's level."""

import time

def get_part_of_day(user_tz_offset, time_now):
    """Return part of day depending on time_now and the user's timzone
    offset value.

    user_tz_offset - integer of user's time zone offset in hours
    time_now - UTC time in seconds

    From  -  To  => part of day
    ---------------------------
    00:00 - 04:59 => midnight
    05:00 - 06:59 => dawn
    07:00 - 10:59 => morning
    11:00 - 12:59 => noon
    13:00 - 16:59 => afternoon
    17:00 - 18:59 => dusk
    19:00 - 20:59 => evening
    21:00 - 23:59 => night
    """
    user_time = time_now + (user_tz_offset*60*60)
    # gmtime[3] is tm_hour
    user_hour = time.gmtime(user_time)[3]

    if 0 <= user_hour < 5:
        return 'midnight'
    elif 5 <= user_hour < 7:
        return 'dawn'
    elif 7 <= user_hour < 11:
        return 'morning'
    elif 11 <= user_hour < 13:
        return 'noon'
    elif 13 <= user_hour < 17:
        return 'afternoon'
    elif 17 <= user_hour < 19:
        return 'dusk'
    elif 19 <= user_hour < 21:
        return 'evening'
    else:
        return 'night'

def choose_greeting(username, level, part_of_day):
    """Return greeting string based on user's level and part of day.

    username - username string
    level - integer of user's level
    part_of_day - string from function `get_part_of_day`
    """

    greetings = {
            'dawn': 'Good early morning',
            'morning': 'Good morning',
            'afternoon': 'Good afternoon',
            'dusk': 'Good afternoon',
            'evening': 'Good evening',
            }

    # Use generic 'Hi' when specific greeting is not implemented
    greeting = greetings.get(part_of_day, 'Hi')

    if level == 0:
        comma = ','
        full_stop = '.'
    elif level == 1:
        comma = ','
        full_stop = '!'
    else:
        comma = ''
        full_stop = '!!'

    return '%s%s %s%s' % (greeting, comma, username, full_stop)

I also wrote tests. This is my first unittest that I wrote. I'm not sure if I'm covering too little, too much, missed corner cases, etc. Am I doing these correctly?

import unittest
from greetings import get_part_of_day, choose_greeting

class TestGreeting(unittest.TestCase):

    def setUp(self):
        pass

    def test_part_of_day_0offset_local_time0(self):
        # 2015-04-01 0:00:00 midnight
        test_time = 1427846400
        user_tz_offset = 0
        part_of_day = get_part_of_day(user_tz_offset, test_time)
        self.assertEqual(part_of_day, 'midnight')

    def test_part_of_day_0offset_local_time13(self):
        # 2015-04-01 13:05:20
        test_time= 1429967120
        user_tz_offset = 0
        part_of_day = get_part_of_day(user_tz_offset, test_time)
        self.assertEqual(part_of_day, 'afternoon')

    def test_part_of_day_minus9offset_local_time5(self):
        # 2012-02-29 05:20:59 (-9)
        # 2012-02-29 14:20:59 UTC
        test_time = 1330525259
        user_tz_offset = -9
        part_of_day = get_part_of_day(user_tz_offset, test_time)
        self.assertEqual(part_of_day, 'dawn')

    def test_part_of_day_minus4offset_local_time22(self):
        # 2030-03-09 22:00:59 (-4)
        # 2030-03-10 02:00:59 UTC
        test_time = 1899338459
        user_tz_offset = -4
        part_of_day = get_part_of_day(user_tz_offset, test_time)
        self.assertEqual(part_of_day, 'night')

    def test_part_of_day_11offset_local_time5(self):
        # 2015-04-25 05:00:00 (+11)
        # 2015-04-24 18:00:00 UTC
        test_time = 1429898400
        user_tz_offset = 11
        part_of_day = get_part_of_day(user_tz_offset, test_time)
        self.assertEqual(part_of_day, 'dawn')

    def test_choose_greeting_testuser_0_dawn(self):
        reply = 'Good early morning, TestUser.'
        self.assertEqual(choose_greeting('TestUser', 0, 'dawn'), reply)

    def test_choose_greeting_testuser_0_midnight(self):
        reply = 'Hi, TestUser.'
        self.assertEqual(choose_greeting('TestUser', 0, 'midnight'), reply)

    def test_choose_greeting_testuser_0_evening(self):
        reply = 'Good evening, TestUser123.'
        self.assertEqual(choose_greeting('TestUser123', 0, 'evening'), reply)

    def test_choose_greeting_testuser_1_afternoon(self):
        reply = 'Good afternoon, Level1User!'
        self.assertEqual(choose_greeting('Level1User', 1, 'afternoon'), reply)

    def test_choose_greeting_testuser_2_dusk(self):
        reply = 'Good afternoon Level2User!!'
        self.assertEqual(choose_greeting('Level2User', 2, 'dusk'), reply)

    def test_choose_greeting_testuser_2_morning(self):
        reply = 'Good morning Level2User!!'
        self.assertEqual(choose_greeting('Level2User', 2, 'morning'), reply)

    def test_choose_greeting_testuser_0_noon(self):
        reply = 'Hi, TestUser.'
        self.assertEqual(choose_greeting('TestUser', 0, 'noon'), reply)

    def test_choose_greeting_testuser_1_noon(self):
        reply = 'Hi, TestUser!'
        self.assertEqual(choose_greeting('TestUser', 1, 'noon'), reply)

    def test_choose_greeting_testuser_2_noon(self):
        reply = 'Hi Level2User!!' # no comma
        self.assertEqual(choose_greeting('Level2User', 2, 'noon'), reply)

if __name__ == '__main__':
    unittest.main()

Output:

----------------------------------------------------------------------
Ran 14 tests in 0.037s

OK

Answer 1

Redundancy in the if-else chain

Your if-else chain will be easier to maintain if you drop the redundant lower bound checks, like this:

if user_hour < 5:
    return 'midnight'
elif user_hour < 7:
    return 'dawn'
elif user_hour < 11:
    return 'morning'
# ... and so on

Btw, another alternative to the if-else chain is to define a table of hour to "part of day" like this:

part_of_day_table = (
    ['midnight'] * 5 +
    ['dawn'] * 2 +
    ['morning'] * 4 +
    # ... and so on
)
assert len(part_of_day_table) == 24

def part_of_day(hour):
    assert 0 <= hour < 24
    return part_of_day_table[hour]

Writing unit tests

First of all, it's great that you're writing unit tests. Keep it up!

In assert statements, it's customary to put the expected (= fixed) value on the left side, and the actual (= computed) value on the right. So instead of this:

    part_of_day = get_part_of_day(user_tz_offset, test_time)
    self.assertEqual(part_of_day, 'dawn')

Prefer this:

    part_of_day = get_part_of_day(user_tz_offset, test_time)
    self.assertEqual('dawn', part_of_day)

Another thing, the way you defined the test times is not ergonomic:

    # 2015-04-01 13:05:20
    test_time= 1429967120

It's good that you added in a comment the meaning of 1429967120, but if I want to tweak it, or add another test with +1 hour, I'll have to figure out a way to convert between human readable times and seconds. It would better if I could readily edit human readable code in the test itself, like this:

    test_time = time.strptime('2015-04-01 13:05', '%Y-%m-%d %H:%M')

or even just:

    test_time = time.strptime('13:05', '%H:%M')

In this version there's no more need for the comment, and easier to tweak and maintain.

Answer 2

You could use a dictionary for your case conditions:

def midnight():
    print "It's midnight!"

def dawn():
    print "Tis dawn"

def morning():
    print 'good morning'

TOD = {00 : midnight,
        01: midnight,
        02: midnight,
        03: midnight,
        04: midnight,
        05: dawn,
        06: dawn,
        07: morning
        }

TEST = 05
TOD[TEST]()

It should find the key in the dictionary and perform the method defined. This would also avoid you from having to have multiple returns in your function which may be frowned upon.

I'm not sure if this is more readable than your code, but it may be a little more OO friendly. I fear this is not any better but would be great to get other peer reviews.

Answer 3

This isn't a complete review, but I've two points for you:

1. Spelling

   Not to be nitpicking, but consistency is important. I've seen accounts of timezone, timzone and time zone. They're comments so it will work just fine, but if you're looking for inconsistencies in code you'll also find the inconsistencies in the comments. Inconsistencies are bad, regardless of whether they're code or comments.

2. Unit tests

   You asked about whether you have too many or not enough unit tests. A good amount of unit tests cover at least 90% of all possible combinations, if possible ALL combinations. You can't cover 90% of all possible time combinations, so you'll just have to pick a few for that.

   Your unit tests could have been more generic:

   def test_choose_greeting_testuser_2_noon(self):
   reply = 'Hi Level2User!!' # no comma
   self.assertEqual(choose_greeting('Level2User', 2, 'noon'), reply)
   

   That's two counts of greeting, two counts of Level2USer (and one testuser_2, why are those inconsistent?) and two counts of noon. That's needless repetition.