HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonModerate

Birthday validity-checking

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
birthdayvaliditychecking

Problem

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 = 1 and day = 1 and day = 1 and day  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.

Solution

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 "", line 1, in ?
    ValueError: year is not within 1900 and 2020
    >>> get_valid_year('2020')
    Traceback (most recent call last):
      File "", line 1, in ?
    ValueError: year is not within 1900 and 2020
    >>> get_valid_year('x')
    Traceback (most recent call last):
      File "", 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.

Code Snippets

valid_year_finding()
valid_month_finding()
valid_day_finding()
birthday_checking()
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)
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')
if year_input > 1900 and year_input <2020:
    year = year_input
else:
    year = 'invalid'
def valid_year_finding():
    global year
    if year_input > 1900 and year_input <2020:
        year = year_input
    else:
        year = 'invalid'

Context

StackExchange Code Review Q#102231, answer score: 13

Revisions (0)

No revisions yet.