5
\$\begingroup\$

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
    print (" ".join(new_read_file))

my_file.close()
\$\endgroup\$
3
\$\begingroup\$

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...:

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you @Calpratt for your advice, I totally get what you mean and this was the exact type of advice I was looking for. I made the change and it worked perfectly! Thank you again for taking a look at my code and giving me some guidance. \$\endgroup\$ – Nate Sep 9 '14 at 23:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.