Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

For some reporting, I need to print a human readable date string for the week covered by the report, using just the standard python module library. So the strings should look like:

Dec 29, 2013 - Jan 4, 2014

Jan 26 - Feb 1, 2014

Jan 19 - 25, 2014

Below is a script I threw together to achieve this. But is there a simpler way to do this?

from datetime import (datetime, date, timedelta)

def week_string(dt):
    (year, week, weekday) = dt.isocalendar()
    week_0 = dt - timedelta(days=weekday)
    week_1 = dt + timedelta(days=(6-weekday))

    month_0 = week_0.strftime("%b")
    day_0 = week_0.strftime("%e").strip()
    year_0 = week_0.strftime("%Y")

    month_1 = week_1.strftime("%b")
    day_1 = week_1.strftime("%e").strip()
    year_1 = week_1.strftime("%Y")

    if year_0 != year_1:
        return "%s %s, %s - %s %s, %s" %(
            month_0, day_0, year_0,
            month_1, day_1, year_1)
    elif month_0 != month_1:
        return "%s %s - %s %s, %s" %(
            month_0, day_0,
            month_1, day_1, year_1)
    else:
        return "%s %s - %s, %s" %(
            month_0, day_0, day_1, year_1)

print week_string(date(2013, 12, 30))
print week_string(date(2014, 01, 30))
print week_string(datetime.date(datetime.now()))

Since the report script is going to be shared with other people, I want to avoid adding dependencies on anything they'd need to install.

share|improve this question
    
datetime is part of the standard library, so they wouldn't need to install anything aside from basic Python – jonrsharpe Jan 24 '14 at 22:17
    
Right, I just mean I'm interested in answers that stick with basic Python modules, as I know there are many non-core modules that handle date formatting. – Bryce Jan 24 '14 at 22:18
up vote 4 down vote accepted

1. Comments on your code

  1. There's no docstring. What does the week_string function do, and how should I call it? In particular, what is the meaning of the dt argument?

  2. You've put your test cases at top level in the script. This means that they get run whenever the script is loaded. It would be better to refactor the test cases into unit tests or doctests.

  3. Your code is not portable to Python 3 because the test cases use the statement form of print.

  4. You import datetime.datetime but don't use it. And you import datetime.date but only use it in the test cases.

  5. There's no need for parentheses in these lines:

    from datetime import (datetime, date, timedelta)
    (year, week, weekday) = dt.isocalendar()
    
  6. The variables are poorly named, as explained by unholysampler.

  7. You format the components (year, month, day) of the dates and then compare the formatted components:

    year_0 = week_0.strftime("%Y")
    year_1 = week_1.strftime("%Y")
    if year_0 != year_1:
        # ...
    

    but it would make more sense to compare the year property of the dates directly:

    if begin.year != end.year:
        # ...
    
  8. The %e format code for srtftime was not defined by the C89 standard and so may not be portable to all platforms where Python runs. See the strftime documentation where this is noted. Also, even where implemented, the %e format code outputs a leading space which doesn't seem appropriate in your case.

    So I would follow unholysampler's technique and use Python's string formatting operation on the day field of the date objects.

  9. Date-manipulating code is often tricky, and you made a mistake in the case where dt is on a Sunday, as pointed out by 200_success. So it would be worth putting in some assertions to check that the manipulations are correct. You can see in the revised code below that I've added assertions checking that begin is on a Sunday, that end is on a Saturday, and that d lies between these two dates.

2. Revised code

from datetime import timedelta

def week_description(d):
    """Return a description of the calendar week (Sunday to Saturday)
    containing the date d, avoiding repetition.

    >>> from datetime import date
    >>> week_description(date(2013, 12, 30))
    'Dec 29, 2013 - Jan 4, 2014'
    >>> week_description(date(2014, 1, 25))
    'Jan 19 - 25, 2014'
    >>> week_description(date(2014, 1, 26))
    'Jan 26 - Feb 1, 2014'

    """
    begin = d - timedelta(days=d.isoweekday() % 7)
    end = begin + timedelta(days=6)

    assert begin.isoweekday() == 7 # Sunday
    assert end.isoweekday() == 6   # Saturday
    assert begin <= d <= end

    if begin.year != end.year:
        fmt = '{0:%b} {0.day}, {0.year} - {1:%b} {1.day}, {1.year}'
    elif begin.month != end.month:
        fmt = "{0:%b} {0.day} - {1:%b} {1.day}, {1.year}"
    else:
        fmt = "{0:%b} {0.day} - {1.day}, {1.year}"

    return fmt.format(begin, end)
share|improve this answer
    
Thanks, this is a very thorough review, and all good advice. Thanks for including the revised (and much improved) code. – Bryce Jan 27 '14 at 19:19

You have a bug: if dt is a Sunday, then it outputs the date range starting from the previous Sunday. That is because datetime.isocalendar() represents the day of week as a number from 1 (= Monday) to 7 (= Sunday), but you want to subtract 0 to 6 days.

>>> print week_string(date(2014, 01, 19))
Jan 12 - 18, 2014

My recommendation:

def week_string(dt):
    # Use underscore to indicate disinterest in the year and week
    _, _, weekday = dt.isocalendar()
    week_0 = dt - timedelta(days=weekday % 7)  # Sunday
    week_1 = week_0 + timedelta(days=6)        # Saturday

Then, I would write the following six lines more compactly:

    day_0, month_0, year_0 = week_0.strftime('%e-%b-%Y').lstrip().split('-')
    day_1, month_1, year_1 = week_1.strftime('%e-%b-%Y').lstrip().split('-')

The rest of it seems fine.

share|improve this answer
    
Thanks for catching the bug, yes you're right. – Bryce Jan 27 '14 at 19:15

Variable Names:

week_0 and week_1 are not helpful names. Firstly, neither contains an object that represents a week.

More importantly, what makes week_0 different from week_1? Seeing numbers used as a suffix makes me think that they are just generic values that could be stored in a list. However, in this case, they are very distinct things. week_start and week_end provide a better description for someone reading the code for the first time.

Repeated Code:

Extracting the month, day, and year values is the same process for both date times. This should be extracted into a function that you call twice.

Building the String: I don't have any issues with this part of the code. But, as I was reviewing this section of the code, I decided that I would write the bulk of the function differently. I would compare the dates in order to choose a format string instead of what is done here.

if week_start.year != week_end.year:
  frmt = "{0:%b} {0.day}, {0.year} - {1:%b} {1.day}, {1.year}"
elif week_start.month != week_end.month:
  frmt = "{0:%b} {0.day} - {1:%b} {1.day}, {1.year}"
else:
  frmt = "{0:%b} {0.day} - {1.day}, {1.year}"

return frmt.format(week_start, week_end)

This way, you don't have to manually extract the date parts. It also makes the format strings easier to read.

share|improve this answer
    
Yes, I chose brevity over clarity on the variable names; guilty! – Bryce Jan 27 '14 at 19:16
    
Huh, I haven't ever used that formatting approach before. Learn something new every day! – Bryce Jan 27 '14 at 19:17
1  
@Bryce: To be honest, I didn't know that curly format strings were that powerful. I stumbled on to it when I was trying to figure out what %e did. But I'm glad I did because the end result turned out very readable. – unholysampler Jan 27 '14 at 19:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.