# defining possible lists
Don't write comments that describe what the code does; we can see the code, we know what it's doing. If you ever need comments, it's where it's otherwise unclear why the code's doing it.
lists = ['cars','favorite foods','family members']
Per the style guide, you should put spaces after the commas:
lists = ['cars', 'favorite foods', 'family members']
x = ['Hyundai','Acura','Honda','Suzuki','Ford']
x is a terrible name for this list (see also: y, z). Names should be more descriptive:
cars = ['Hyundai', 'Acura', 'Honda', 'Suzuki', 'Ford']
But given that lists is supposed to refer to the three lists below it, why not use a dictionary instead:
lists = {
'cars': ['Hyundai', 'Acura', 'Honda', 'Suzuki', 'Ford'],
...
}
def listselection():
print('The following lists exist within the system. Which would you like to see?')
print(lists)
The style guide recommends snake_case names for functions. It's helpful to include a docstring when you define a function. Given the above switch to a dictionary, we need to change the second line slightly:
def list_selection():
"""Show the user the selection of available lists."""
print('The following lists exist within the system. Which would you like to see?')
print(list(lists))
Note that by default you get a dictionary's keys when you iterate over it, so list(lists) == ['cars', ...].
def errorchecking():
global option
while True:
try:
option = str(input())
except:
print('Please type letters, not numbers.')
continue
if option == 'cars':
break
elif option == 'favorite foods':
break
elif option == 'family members':
break
else:
print('Please re-enter your selection')
return option
Using a while True loop to take and validate input is a good idea, but there are a few problems here:
Why do you use global option? You return option at the end of the function, which is the way you ought to provide it to other functions. Using global state is a bad pattern, get out of the habit of doing it.
Your error checking doesn't actually do anything. The result of input(...) is always a string, str(input(...)) will never cause an exception. Also your error checking is too broad; bare except is a bad idea, you should be specific about the kind of errors you expect and allow unexpected exceptions to get raised up. "Errors should never pass silently. Unless explicitly silenced."
This part:
if option == 'cars':
break
elif option == 'favorite foods':
break
elif option == 'family members':
break
else:
print('Please re-enter your selection')
is not very DRY (Don't Repeat Yourself); we already have those options, they're the keys to our dictionary (or the values of your list in the original). Therefore we could write:
if option in lists:
break
else:
print('Please re-enter your selection')
With all of the above changes the function is now a lot more concise:
def error_checking():
"""Take valid input from the user."""
while True:
option = str(input())
if option in lists:
return option # this will also end the loop
print('Please re-enter your selection')
Now it's clear that error_checking doesn't really describe what this function does; that's only part of its job, but the name should tell you how to use it. I would call it something like get_valid_input.
def listconcatenation():
global spam
global listitem
listitem = 'Here are the items in my list: '
if option == 'cars':
spam = x
elif option == 'favorite foods':
spam = y
elif option == 'family members':
spam = z
for i in spam:
if spam.index(i) < (len(spam)-1):
listitem = listitem + i + ', '
else:
listitem = listitem + 'and ' + i + '.'
return listitem
Same things again: put underscores in the names; remove the global state (make option a parameter instead, you already return listitem). But there are three more specific improvements:
The dictionary we introduced above means you can go from this:
if option == 'cars':
spam = x
elif option == 'favorite foods':
spam = y
elif option == 'family members':
spam = z
to this:
item_list = lists[option] # note also a more descriptive name
That said, why do this inside list_concatenation? That makes that function very tightly coupled to the place where you're currently using it, it's not reusable at all. Instead, why not pass in the selected list, rather than the option name, as a parameter?
Similarly including things like 'Here are the items in my list: ' and the trailing full stop '.' within the function reduce its reusability.
In terms of looping in Python:
for i in item_list:
if item_list.index(i) < (len(item_list)-1):
listitem = listitem + i + ', '
else:
listitem = listitem + 'and ' + i + '.'
The i would usually suggest an index, perhaps item would be a better name. If you want the index, rather than using spam.index(item) you can enumerate over the list:
for index, item in enumerate(item_list):
if index < (len(item_list) - 1):
This is more efficient as you're only passing over the list once, rather than every time you need the index. Note the additional whitespace, again per the style guide.
With all of the above, we have to change the invocation of the functions:
list_selection()
item_list = lists[take_valid_input()]
result = list_concatenation(item_list)
print('Here are the items in my list: {}.'.format(result))
Now the data is moving through the functions in a logical way, rather than being updates to the global module state, and list_concatenation is a very generic, reusable function.
I might also be inclined to move list_selection inside take_valid_input, or even write a more generic function for user input that:
- Takes a dictionary of options;
- Displays the option names to the user;
- Loops until they've entered one of the option names; and
- Returns the one they've entered.
