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.

• d_dd: I will definitely use your suggestion about the test_time. The time_of_day_table tuple is a neat method!Thank you for the review.

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.

• d_dd: I never thought about using a dictionary with function values. It would change some of the code if I had to return a function but I will play around with this. Thanks for the idea!

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.

• d_dd: I didn't even notice the discrepancies until you pointed them out, and I thought I looked over it a few times. I agree with the tests that they are a bit disorganized. The last part, I was trying to just test one change at a time, which is the reason for all the repetition. Thank you for the review!