Birthday validity-checking

Question

I have written code to check any birthday input's validity. As I am new in programming, and after going through several debugging steps, the code became very ugly.

month_dict = {'jan':'January',
          'feb':'February',
          'mar':'March',
          'may':'May',
          'jul':'July',
          'sep':'September',
          'oct':'October',
          'dec':'December',
          'apr':'April',
          'jun':'June',
          'aug':'August',
          'nov':'November'}

day = int(raw_input ('Enter your birth day: '))
month = raw_input ("Enter your birth month: ")
year_input = int (raw_input ('Enter your birth year: '))

days_31 = ['jan', 'mar', 'may', 'jul', 'aug', 'oct', 'dec']
days_30 = ['apr', 'jun', 'sep', 'nov']
days_28 = ['feb']

def valid_day_finding ():
    global valid_day
    if month_name in days_31:
        if day > 0 and day < 32:
            valid_day = day
        else:
            valid_day = 'invalid'
    elif month_name in days_30:
        if day >= 1 and day <= 30:
            valid_day = day
        else:
            valid_day = 'invalid'
    elif month_name in days_28:
        if year != 'invalid':
            if (year % 4 == 0 and year % 100 != 0) or (year % 400 == 0):
                if day >= 1 and day <= 29:
                    valid_day = day
                else:
                    valid_day = 'invalid'
            else:
                if day >= 1 and day <= 28:
                    valid_day = day
                else:
                    valid_day = 'invalid'
        else:
            valid_day = 'invalid'
    else:
        valid_day = 'invalid'             
def valid_month_finding():   
    global month_name
    if month in month_dict.keys():
        month_name = month
    else:
        month_name = 'invalid'
def valid_year_finding():
    global year
    if year_input > 1900 and year_input <2020:
        year = year_input
    else:
        year = 'invalid'
def birthday_checking():
    if valid_day != 'invalid' and month_name != 'invalid' and year != 'invalid':
        print 'your birthdate is %d - %s - %d' % (valid_day, month_dict[month_name], year)
    else:
        print 'Your Birthday is invalid'
valid_year_finding()    
valid_month_finding()
valid_day_finding()
birthday_checking()

This code is very inefficient. How can I improve this code?

I know that there are built-in functions to check this, but I am trying to learn coding, which is why I am giving this a try.

Answer 1

Naming and using functions

These are all very bad function names:

valid_year_finding()
valid_month_finding()
valid_day_finding()
birthday_checking()

Think of functions as actions. Typically their names should include a verb, take an input, and either return an output, or modify state. Consider this rephrasing and usage:

try:
    year = get_valid_year(year_input)
    month = get_valid_month(month_input)
    day = get_valid_day(year, month, day_input)
    validate_birthday(year, month, day)

    print('Your birthday is {} - {} - {}'.format(year, month, day))

except ValueError as e:
    print('Invalid birthday: ' + e.message)

The get_* methods should return valid values, or else raise an exception if the input is invalid. This way if a problem is found with year, there's no need to process month.

Notice that above I pass year and month as parameters to get_valid_day. This usage makes sense, as you cannot validate a day without knowing the year and the month.

Example implementation:

def get_valid_year(year_input):
    year = int(year_input)
    if 1900 < year < 2020:
        return year
    raise ValueError('year is not within 1900 and 2020')

Formatting

There are serious formatting issues with this code. Please read and follow the Python style guide.

A variable should have one type

For example after these statements, year might end up as either integer or string:

if year_input > 1900 and year_input <2020:
    year = year_input
else:
    year = 'invalid'

Avoid the global keyword, and global variables

It's too hard to avoid using global. For example instead of this:

def valid_year_finding():
    global year
    if year_input > 1900 and year_input <2020:
        year = year_input
    else:
        year = 'invalid'

You could reorganize the code like this:

def valid_year_finding():
    if year_input > 1900 and year_input < 2020:
        return year_input
    else:
        return 'invalid'

year = valid_year_finding()

We got rid of the global keyword, but the year_input global variable still remains. Let's make that a parameter of the function instead:

def valid_year_finding(year_input):
    if year_input > 1900 and year_input < 2020:
        return year_input
    else:
        return 'invalid'

year = valid_year_finding(year_input)

Avoid free variables

If a variable is used in a code block but not defined there, it is a free variable. In this function, year_input is a free variable:

def valid_year_finding():
    if year_input > 1900 and year_input < 2020:
        return year_input
    else:
        return 'invalid'

Free variables are acceptable when their scope is small and obvious. In the posted code, year_input is defined in the global scope, making it a global variable, which should be avoided.

Use the chained comparison operator

This condition can be simplified:

   if year_input > 1900 and year_input < 2020:

Using the chain comparison operator:

    if 1900 < year_input < 2020:

Use the right data structures

Instead of lists:

days_31 = ['jan', 'mar', 'may', 'jul', 'aug', 'oct', 'dec']
days_30 = ['apr', 'jun', 'sep', 'nov']
days_28 = ['feb']

These should be sets:

days_31 = {'jan', 'mar', 'may', 'jul', 'aug', 'oct', 'dec'}
days_30 = {'apr', 'jun', 'sep', 'nov'}
days_28 = {'feb'}  # granted, for this one, list or set doesn't matter

Use doc tests

Doc tests can greatly help you test your implementations, here's an example:

def get_valid_year(year_input):
    """
    >>> get_valid_year('1901')
    1901
    >>> get_valid_year('1900')
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
    ValueError: year is not within 1900 and 2020
    >>> get_valid_year('2020')
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
    ValueError: year is not within 1900 and 2020
    >>> get_valid_year('x')
    Traceback (most recent call last):
      File "<stdin>", line 1, in ?
    ValueError: invalid literal for int() with base 10: 'x'
    """
    year = int(year_input)
    if 1900 < year < 2020:
        return year
    raise ValueError('year is not within 1900 and 2020')

If you have this code in a script called bday.py, you could run the doc tests with:

python -m doctest bday.py

If the tests pass, it outputs nothing. Otherwise it prints an informative report of what went wrong.

Answer 2

• It is good practice to return a boolean (True/False) for functions that check a "yes or no" thing.
• You could reduce your code with a dictionary that links month names to their number of days. This makes it so that you only have to use one if statement to check everything.

Hope it helps :)

Answer 3

As as already been mentioned, you should avoid global variables and use parameters instead, it will make the flow of information within your program much easier to follow.

Apart from this point there are two other issues:

• the NIH syndrom (Not Invented Here): there are libraries already existing to manipulate dates and times, use them!
• the hardcoded validity range

With that in mind, I present to you the Python datetime library. It's built-in (comes with your Python interpreter) and already handles many of the dates functionalities. As a result, it will handle date validation by itself, and the only think that you are left with is:

• massaging the input to fit into datetime
• validating that the presumed birthday date is likely

Let's code!

# Let's convert the input month into its number [1, 12]
def get_month_number(month):
    months = ["jan", "feb", "mar", "apr", "may", "jun",
              "jul", "aug", "sep", "oct", "nov", "dec"]

    if month not in months:
        raise RuntimeError("Invalid month: " + month + ", pick among " + str(months))

    return months.index(month) + 1 # Index is 0-based

So, let's dissect this:

• the name of the function is indicative of its function
• it is based on the list class, which handles look-up for us
• when reporting the error, we take care of indicating precisely what are the valid values; it makes it easier for the user or next maintainer
• there is a slight trick at the end: list.index is 0-based while we want the month number which is 1-based, so we add a comment next to the + 1 to explain why it's there

This was actually the most difficult part (asking directly for the month number would be easier), so now we get it easy:

def validate_birthday(year, month, day):
    month_number = get_month_number(month)

    date = datetime.datetime(int(year), month_number, int(day))

    now = datetime.datetime.now()

    if now - 130 < date < now: return

    raise RuntimeError("Invalid birthdate: " + str(date))

And let's dissect this one two:

• the use of get_month_number is self explanatory, I hope
• next, we create a datetime object using its constructor; if the date is invalid (30th of February for example) then it will throw an exception explaining the issue (ValueError: day is out of range for month)
• then, we obtain the current date
• and we are finally down to checking whether the birthday is likely or not: is it in the past, but not too much?

Answer 4

Functions should take arguments, for example

def valid_year_finding(year):
      # Removs the global year line, the rest is the same

And call them like:

valid_year_finding(year)

A function that performs logic only on his parameter is easier to understand than another that processes arbotrary global vars.

Answer 5

I agree with the other answers. You have an overly complicated way of finding the days in a month when a dictionary would serve much better. In general if you find yourself doing a chain of if elifs then think about if you could use a dictionary. So first I'd suggest storing the day information like this:

daysInMonth = {'jan':31, 'feb':28, 'mar':31, 'apr':30, 'may':31, 'jun':30,
               'jul':31, 'aug':31, 'sep':30, 'oct':31, 'nov':30, 'dec':31}

Now you can just pass the month and get the amount of days in that month.

def get_valid_day(day, month):
    maxDays = daysInMonth[month]

    if 0 < day < maxDays:
        return day

Except it's not that easy because of February. You should still include a check before that to determine if it is February and a leap year. And if it is, then change the maxDays value, but still let the normal check run after that. I'm also not including a test for if year is valid because you should have raised an error by now if it's not.

def get_valid_day(day, month):
    maxDays = daysInMonth[month]

    # Is this a February in a leap year?
    if month == 'feb' and ((year % 4 == 0 and year % 100 != 0)
                            or (year % 400 == 0)):
        maxDays = 29

    if 0 < day <= maxDays:
        return day