patternpythonModerate
Birthday validity-checking
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.
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.
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:
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:
The
or else raise an exception if the input is invalid.
This way if a problem is found with
there's no need to process
Notice that above I pass
This usage makes sense,
as you cannot validate a day without knowing the year and the month.
Example implementation:
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,
Avoid the
It's too hard to avoid using
You could reorganize the code like this:
We got rid of the
but the
Let's make that a parameter of the function instead:
Avoid free variables
If a variable is used in a code block but not defined there, it is a free variable.
In this function,
Free variables are acceptable when their scope is small and obvious.
In the posted code,
making it a global variable, which should be avoided.
Use the chained comparison operator
This condition can be simplified:
Using the chain comparison operator:
Use the right data structures
Instead of lists:
These should be sets:
Use doc tests
Doc tests can greatly help you test your implementations,
here's an example:
If you have this code in a script called
you could run the doc tests with:
If the tests pass, it outputs nothing.
Otherwise it prints an informative report of what went wrong.
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 variablesIt'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 matterUse 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.pyIf 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.