5
\$\begingroup\$

I am a beginner with only about 2 months under my belt. I'm learning Python 3 using the Automate the Boring Stuff with Python manual. In the chapter on reading and writing files one of the exercises is to make a program that would fill in a mad lib that was read from a file. It works, but it's not very elegant. Can anyone offer me advice on how to better do this task or a better direction to go?

# madLibs.py

import re, os

# define regex
verbRegex = re.compile(r'verb', re.I)
nounRegex = re.compile(r'noun', re.I)
adjectiveRegex = re.compile(r'adjective', re.I)

# change directory 
os.chdir('C:\\MyPythonScripts')

# read contents of madLib.txt and store it in variable madLib
madLibFileObject = open('madLib.txt')
madLib = madLibFileObject.read()

y = 0
newMadLib = []

for word in madLib.split():
    newMadLib.append(word)
    if verbRegex.search(word) != None:
        x = input('Enter VERB:  ')
        newMadLib[y] = x
    elif nounRegex.search(word) != None:
        x = input('Enter NOUN:  ')
        newMadLib[y] = x
    elif adjectiveRegex.search(word) != None:
        x = input('Enter ADJECTIVE:  ')
        newMadLib[y] = x
    y += 1

newMadLib = (' ').join(newMadLib)

print(madLib)
print (newMadLib)
\$\endgroup\$
4
\$\begingroup\$

It is unnecessary to change the current directory in your python script. Instead you can just open the file with an absolute path - i.e.

madLibFileObject = open("C:\\MyPythonScripts\\madLib.txt")`


To avoid leaking files (madLibFileObject), it it a good idea to use a with statement whenever you open a file, which takes care of automatically closing it for you:

with open("C:\\MyPythonScripts\\madLib.txt") as madLibFileObject:
     madLib = madLibFileObject.read()


Using three regular expressions that each match one word is overkill for this task. It is instead better to use word.lower() == "verb"

for word in madLib.split():
    newMadLib.append(word)
    if word.lower() == "verb":
        x = input('Enter VERB:  ')
        newMadLib[y] = x
    elif word.lower() == "noun":
        x = input('Enter NOUN:  ')
        newMadLib[y] = x
    elif word.lower() == "adjective":
        x = input('Enter ADJECTIVE:  ')
        newMadLib[y] = x

Then, there is clearly quite a bit of code duplication in the above loop, so it should be shortened using a list:

PARTS_OF_SPEECH = ["verb", "adjective", "noun"]
for word in madLib.split():
    if word.lower() in PARTS_OF_SPEECH:
        newMadLib[y] = input("Enter " + word.upper())


Your y counter variable is a reimplementation of the enumerate built-in function, in addition to being poorly named. index would be a better name:

for index, word in enumerate(madLib.split()):

Full code:

PARTS_OF_SPEECH = ["verb", "noun", "adjective"]
with open("C:\\MyPythonScripts\\madLib.txt") as madLibFileObject:
     madLib = madLibFileObject.read()
newMadLib = []
for index, word in enumerate(madLib.split()):
     if word.lower() in PARTS_OF_SPEECH:
        newMadLib[index] = input("Enter " + word.upper())
newMadLib = ' '.join(newMadLib)
print(madLib)
print(newMadLib)
\$\endgroup\$
1
\$\begingroup\$

Your loop is clunky:

y = 0
newMadLib = []

for word in madLib.split():
    newMadLib.append(word)
    if verbRegex.search(word) != None:
        x = input('Enter VERB:  ')
        newMadLib[y] = x
    # ...
    y += 1

What you are doing is deciding whether to append the word to the list, or whether to replace the word with input from the user.

In both cases you have a word to append to the list. So I suggest restructuring so that you can just do an .append and you don't have to keep the y variable:

for word in madLib.split():
    if ...
        newMadLib.append(...)
    ...
    else:
        newMadLib.append(word)
\$\endgroup\$

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.