5
\$\begingroup\$

I'm brand new at Python, but I've been working in general at granulating my functions and being as self-readable as possible without suffering readability. This CodingBat question is a little interesting because it demands a function with a lot of repetitive case checks. I decided, rather than having a bunch of nested ifs, it might be better to break them out into separate functions, but I'm not sure if this actually goes -against- my goal of better readability. However, I decided to break out is_weekday because the number encoding wasn't immediately clear. I also wasn't sure if it'd be better to have alarm_time(is_weekday(day), '10:00', 'off') or do alarm_time(day, '10:00', 'off') and have alarm_time handle the is_weekday(day) check. I decided the first way because it seemed there was a separation of concern issue otherwise, but I'm not sure.

I was at first hesitant to ask because this might be gray-space into asking a general opinion, but I'm specifically asking in terms of this exercise to use in similar situations.

Given a day of the week encoded as 0=Sun, 1=Mon, 2=Tue, ...6=Sat, and a boolean indicating if we are on vacation, return a string of the form "7:00" indicating when the alarm clock should ring. Weekdays, the alarm should be "7:00" and on the weekend it should be "10:00". Unless we are on vacation -- then on weekdays it should be "10:00" and weekends it should be "off".

def is_weekday(day):
    return 1 <= day <= 5

def alarm_time(is_weekday, weekday_time, weekend_time):
  if is_weekday:
    return weekday_time
  return weekend_time

def alarm_clock(day, vacation):
  if vacation:
    return alarm_time(is_weekday(day), '10:00', 'off')
  return alarm_time(is_weekday(day), '7:00', '10:00')


def test_fun(fun):
    cases = [[1, False, '7:00'], [5, False, '7:00'], [0, False, '10:00'], [6, False, '10:00'], [0, True, 'off'], [6, True, 'off'], [1, True, '10:00'], [3, True, '10:00'], [5, True, '10:00']]

    print('{:^30}     {:^11}'.format('Expected', 'Run'))
    for case in cases:
        funstr   = '{}({}, {})'.format(fun.__name__, case[0], case[1])
        result   = fun(case[0], case[1])
        expected = case[-1]
        success  = result == expected

        print('{:21} -> {:5} ==> {:5}  {}'.format(funstr, expected, result, success))

The test cases were positive:

>>> test_fun(alarm_clock)
           Expected                    Run    
alarm_clock(1, False) -> 7:00  ==> 7:00   True
alarm_clock(5, False) -> 7:00  ==> 7:00   True
alarm_clock(0, False) -> 10:00 ==> 10:00  True
alarm_clock(6, False) -> 10:00 ==> 10:00  True
alarm_clock(0, True)  -> off   ==> off    True
alarm_clock(6, True)  -> off   ==> off    True
alarm_clock(1, True)  -> 10:00 ==> 10:00  True
alarm_clock(3, True)  -> 10:00 ==> 10:00  True
alarm_clock(5, True)  -> 10:00 ==> 10:00  True
\$\endgroup\$
5
\$\begingroup\$

Testing

I find it weird that alarm_clock is passed to test_fun as a parameter, but the cases are hard-coded within test_fun. They should either both be hard-coded, or both passed as parameters.

Stylistically, cases would be better as a list of tuples. Tuples have a connotation of being non-homogeneous collections of uniform length (like a single row in a database table). Lists have a connotation of being homogeneous collections of undetermined length.

The way you split case[0], case[1], and case[-1] is awkward. I would put the singular expected result first, then use tuple unpacking.

For clarity, you should print repr(param) instead of param.

Here's how I would write it:

def test_fun(fun, cases):
    print('{:^30}     {:^11}'.format('Expected', 'Run'))
    for expected, *params in cases:
        funstr = '{}({})'.format(fun.__name__, ', '.join(repr(p) for p in params))
        result = fun(*params)
        success = result == expected
        print('{:21} -> {:5} ==> {:5}  {}'.format(funstr, expected, result, success))

test_fun(alarm_clock, [
  ('7:00', 1, False),
  ('7:00', 5, False),
  ('10:00', 0, False),
  …
])

Better yet, consider dropping test_fun altogether and using a doctest instead.

Implementation

For a simple problem like this, there are many ways to write it, and choosing the "best" one is a matter of taste. Personally, I would prefer to keep it simple so that you don't have to jump back and forth among three functions to understand the code.

def alarm_clock(day, vacation):
    """
    Given a day of the week encoded as 0=Sun, 1=Mon, 2=Tue, ...6=Sat, and a
    boolean indicating if we are on vacation, return a string of the form
    "7:00" indicating when the alarm clock should ring. Weekdays, the alarm
    should be "7:00" and on the weekend it should be "10:00". Unless we are on
    vacation -- then on weekdays it should be "10:00" and weekends it should
    be "off".

    >>> alarm_clock(1, False)
    '7:00'
    >>> alarm_clock(5, False)
    '7:00'
    >>> alarm_clock(0, False)
    '10:00'
    """
    weekday = 1 <= day <= 5
    return ( '7:00' if weekday and not vacation else
            '10:00' if weekday ==      vacation else
            'off')

The CodingBat editor seems to encourage two spaces per level of indentation. PEP 8, the official Python style guide, says that you should use four spaces.

\$\endgroup\$
  • 1
    \$\begingroup\$ Very fine answer, I would prefix boolean variables with is_ as it makes reading more intuitive and would nest ternaries to re-create the 4-state table that seems implied in the question (is_weekday x is_holyday) ignoring the repetition of 10:00 in the result to more closely follow the spec. \$\endgroup\$ – Caridorc Aug 20 '16 at 13:50

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.