4
\$\begingroup\$

I'm a complete beginner in Python and programming in general. I made a program for Spotify's Best Before puzzle. It was accepted. I have looked a litle around on internet and looked at other solutions to the problem, and everyone I have seen have importet several modules, inclusive the Calendar module. I understand this is probably a good solution, but I wanted to make everything myself as a practice.

I would really appreciate all tips and hint, but mainly without haveing to import code. It's primarily the printer(a) and det dataMaker() that needs modification.

normYear = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
leapYear = [31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
answerList = []

u''' Check if any of the integers are years '''

def yearCheck():
    for x in xrange(0, 3):
        a = dataList[x]
        if len(a) > 2:
            if not len(a) == 4 and int(a) in xrange(2000,3000):
                if int(a) in xrange(100,1000):
                    dataList[x] = int(a) + 2000
                else:
                    print data + u" is illegal"

u''' Make integers and sort '''

def integer():
    for x in xrange(0, 3):
        dataList[x] = int(dataList[x])
    dataList.sort()

u''' Check for possible leap years '''

def leapYears():
    global leapList
    leapList = []
    for x in xrange(0, 3):
        if dataList[x] % 4 == 0:
            if dataList[x] % 100 == 0:
                if dataList[x] % 400 == 0:
                   leapList.append(x)
            else:
                leapList.append(x)

u''' Changes year type '''

def defYear(a):
    global xYear
    if a in leapList:
        xYear = leapYear
    else:
        xYear = normYear

u''' Printer '''

def printer(a):
    if dataList[a] < 2000:
        dataList[a] += 2000
    year = dataList[a]
    del dataList[a]
    if not dataList[0] == 0:
        month = dataList.pop(0)
        day = dataList.pop(0)
        answerList.append(unicode(year))
        answerList.append(unicode(u'%02d' % month))
        answerList.append(unicode(u'%02d' % day))
        print u'-'.join(answerList)
    else:
        print data + u" is illegal"

u''' Looks for legal dates, first [Y<M<D] then [M<Y<D] then [M,D,Y] '''

def dateMaker():
    for x in xrange(0,4):
        defYear(x)
        if x == 0:
            if dataList[1] <= 12 and dataList[2] <= xYear[dataList[1]-1]:
                printer(x)
                break
        elif x == 1:
            if dataList[0] <= 12 and dataList[2] <= xYear[dataList[0]-1]:
                printer(x)
                break
        elif x == 2:
            if dataList[0] <= 12 and dataList[1] <= xYear[dataList[0]-1]:
                printer(x)
                break
        else:
            print data + u" is illegal"

u''' Program '''

data = raw_input()
dataList = data.split(u"/")
yearCheck()
integer()
leapYears()
dateMaker()
\$\endgroup\$
4
\$\begingroup\$
  1. You have unicode strings as comments. Just use comments.
  2. You rely on manipulating global state, not a good idea. Instead, have function return things. You don't use any return statements anywhere in your code.
  3. You use not a == b instead of a != b in several places
  4. You use a in xrange(b,c) where b <= a < c would be better
  5. All of your for loops use xrange, usually you want to iterate over the container directly
  6. Your yearCheck function doesn't do anything. The if condition is impossible to satisfy. It looks like you discovered this, because you've implemented the logic down in the printer function.
  7. You delay converting the data components to integer, this only serves to complexicate your code.
  8. You generate a list of leap years, but you only ever check the years 0, 1, 2 and 3.
  9. You keep the data inside the list the whole time. Code is easier to follow if you don't do that. Pull the data out of lists into local variables rather then indexing into the list.

Here is a reworking of your approach:

NORMAL_MONTHS = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]
LEAP_MONTHS = [31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]

def get_month_lengths(year):    
    """
    Returns appropriate months for a particular year
    """
    if year % 4 == 0:
        if year % 100 == 0:
            if year % 400 == 0:
                return LEAP_MONTHS
        else:
            return LEAP_MONTHS
    return NORMAL_MONTHS


def print_date(year, month, day):
    """
    Print the given date
    """
    if year < 2000:
        year += 2000
    print u'%04d-%02d-%02d' % (year, month, day)

# Looks for legal dates, first [Y<M<D] then [M<Y<D] then [M,D,Y] 

def dateMaker(data):
    """
    Try different ways of interpretating the date
    """
    a, b, c = data

    for year, month, day in [(c,b,a), (c,a,a), (b,a,c)]:
        month_lengths = get_month_lengths(year)
        if month <= 12 and day <= month_lengths[month - 1]:
            print_date(year, month, day)
            return

    print unicode(data) + u" is illegal"


data = sorted(map(int, raw_input().split(u"/")))
dateMaker(data)
\$\endgroup\$
  • \$\begingroup\$ Removed print(year, month, day). Changed order of a,b,c in for function. Added 0 to month lists to make it right. \$\endgroup\$ – Martin Hallén Jan 24 '12 at 23:28
  • \$\begingroup\$ @mart0903, I don't like adding extra padding to lists like that. I subtracted one from the month instead. \$\endgroup\$ – Winston Ewert Jan 24 '12 at 23:36
  • \$\begingroup\$ That's good. But another problem is if you type in 0/0/2 that gives out 2002/00/00 or 0/34/11 that gives error \$\endgroup\$ – Martin Hallén Jan 25 '12 at 1:49
  • \$\begingroup\$ @mart0903, true enough. I'd probably check for zeros first off and then report an error to the user. \$\endgroup\$ – Winston Ewert Jan 26 '12 at 0:29
3
\$\begingroup\$

First of all, the code is hard to read because :

  • almost all the functions access global values. it is much better to send a value as a parameter, it makes the flow explicit.
  • the list is modified in place; again this makes hard to know which version of the list a function is manipulating. create new version for each pass of the processing.
  • Many python idioms are not used ( like the for x in list)
\$\endgroup\$

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.