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

Madlibs Program

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

Problem

I just started to learn Python, and wrote this Madlib program that works the way I intended it to, so I do not need help debugging, just some advice on tips to improve the code or make it simpler.

Program and Madlibs text files on Google Drive
Madlibs Reader

```
#Date 8SEP14

#introduction and file select
print ("Welcome to Madlibs!")
print ("Please select from the list below which Madlib Adventure you would like to do! ")

#display list of available madlibs
from os import listdir
for each in listdir():
if each.endswith('txt'):
print (each)
file_choice = input("What Madlib would you like to do? ")

#if input is not good then repeats until a good input is entered
while file_choice not in listdir():
file_choice = input("Please try again and don't forget the .txt at the end ")

#opening file and setting variables
with open(file_choice,"r",encoding = "utf-8") as my_file:
word_types = ["Verb","Adjective","Noun","Last-Name","Illness","Number","Place","Silly-Word","Body-Part","Past-Tense-Verb","Adverb","Verb-Ending-ING","Noun-Plural","Celebrity","Movie","Verb-Ending-ED","Mean-Nickname","Liquid","Cute-Animal-Plural","Article-of-Clothing","Celebrity-1","Celebrity-2"]
user_inputs = []
user_inputs_index = []
my_read_file = my_file.read()
new_read_file = my_read_file.split()
x = 0
for item in new_read_file:
if item in word_types and user_inputs_index == []:
user_inputs.append(item)
user_inputs_index.append(new_read_file.index(item))
else:
if item in word_types:
user_inputs.append(item)
user_inputs_index.append(new_read_file.index(item,(int(user_inputs_index[-1])+1)))
for each in user_inputs:
user_inputs[x] = input("Please Enter a(n) " + each + ": ")
x += 1
z = 0
for word in user_inputs:
y = int(user_inputs_index[z])
new_read_file[y] = word
z += 1
p

Solution

One thing I notice here is that you don't follow the DRY principle. Don't Repeat Yourself.

Always make an attempt to not repeat yourself because it a) cuts down on code, b) makes code easier to modify, and c) usually runs more efficiently. for example.

for item in new_read_file:
    if item in word_types and user_inputs_index == []:
        user_inputs.append(item)
        user_inputs_index.append(new_read_file.index(item))
    else:
        if item in word_types:
            user_inputs.append(item)
            user_inputs_index.append(new_read_file.index(item,(int(user_inputs_index[-1])+1)))


This function checks item in word_types two times in a row (which is inefficient), and in each case you do user_inputs.append(item) when you would do it regardless (which makes it harder to modify).

Try changing it to:

for item in new_read_file:
    if item in word_types:
        user_inputs.append(item)
        if user_inputs_index == []:
            user_inputs_index.append(new_read_file.index(item))
        else:
            user_inputs_index.append(new_read_file.index(item,(int(user_inputs_index[-1])+1)))


PS instead of writing else: if...: you can simply write elif...:

Code Snippets

for item in new_read_file:
    if item in word_types and user_inputs_index == []:
        user_inputs.append(item)
        user_inputs_index.append(new_read_file.index(item))
    else:
        if item in word_types:
            user_inputs.append(item)
            user_inputs_index.append(new_read_file.index(item,(int(user_inputs_index[-1])+1)))
for item in new_read_file:
    if item in word_types:
        user_inputs.append(item)
        if user_inputs_index == []:
            user_inputs_index.append(new_read_file.index(item))
        else:
            user_inputs_index.append(new_read_file.index(item,(int(user_inputs_index[-1])+1)))

Context

StackExchange Code Review Q#62356, answer score: 3

Revisions (0)

No revisions yet.