3
\$\begingroup\$

I'm learning Python and I'm building a basic madlib generator. The code below works as expected. I'm just wondering if it uses best practice / can be made better.

def intro():
    print("Welcome to our madlib generator!")
    print("We'll ask you for a serious of words, you'll give us the words, and "
          "we'll make a dope ass story out of it!")
    print("Let's get some info from you first.")
    print("Select the type of madlib you want. ")
    madlib_type = input("1. Short Story\n2. Poem\n")
    return madlib_type


def user_input():
    name = input("Enter a name: ")
    color = input("Enter a color: ")
    animal = input("Enter an animal: ")
    food = input("Enter a food: ")
    item = input("Enter an item: ")
    adjective = input("Enter an adjective: ")
    noun1 = input("Enter a plural noun: ")
    noun2 = input("Enter any noun: ")
    return [name, color, animal, food, item, adjective, noun1, noun2]


class MadlibGenerator:
    def __init__(self, *args):
        self.name = args[0].title()
        self.color = args[1]
        self.animal = args[2]
        self.food = args[3]
        self.item = args[4]
        self.adjective = args[5]
        self.noun1 = args[6]
        self.noun2 = args[7]

    def short_story_generator(self):
        print(f"There once was a person named {self.name}. {self.name} LOVED "
              f"{self.animal}s. They were {self.name}'s favorite animal.")
        print(f"One day, {self.name} was walking down the street wearing "
              f"{self.color} pants and a {self.color} shirt. In fact, all of "
              f"{self.name}'s ")
        print(f"clothes were {self.color}! In one of {self.name}'s hand, "
              f"there was a bright green {self.item}. In the other, there was "
              f"a big {self.food} that {self.name} was waiting to eat!")

    def poem_generator(self):
        print(f"Roses are {self.adjective}")
        print(f"{self.noun1} are blue")
        print(f"I can't start my day without {self.noun2}")


madlib_type = intro()
madlib_input = user_input()
madlib = MadlibGenerator(madlib_input[0], madlib_input[1], madlib_input[2],
                         madlib_input[3], madlib_input[4], madlib_input[5],
                         madlib_input[6], madlib_input[7])
if madlib_type == "1":
    madlib.short_story_generator()
if madlib_type == "2":
    madlib.poem_generator()
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

You're asking for a lot of words you don't use. It might be nicer to only ask the user for relevant words. Maybe instead of user_input taking no arguments, it could take a list of descriptions like user_input(["a name", "a color", "a different color"]) and ask based on those?

I don't see why the two story generators live in the same class like that. Passing the same data to multiple generators is probably not very useful very often. The functions can just as easily take the words as arguments themselves instead. This doesn't make the idea of a MadlibsGenerator class inappropriate, but the two functions belong to different generators, and the functions' input doesn't need to be directly tied to a generator at all.

The generators also do two different things each - they both generate the stories and print them. Usually, it would be better to do those separately, in case you maybe want to do something else with the stories as well.

You don't check if the user provides valid input when selecting which generator to use. If I'm asked to pick 1 or 2 and enter 3, I'd probably expect an error message and maybe an opportunity to try again. Instead, the program just keeps going as usual, except I don't get a fun story at the end.

Some day, someone else might want to use these functions (or maybe you'll want to use them for something else). They'll probably try importing your file as a module. When they do that, the entire file is run, which right now means the entire madlibs program. That's probably not what they wanted. You can prevent that by placing the program logic inside an if __name__ == "__main__": block, so that it doesn't execute when the file is imported.

Branching on a string to figure out which generator to use is a bit ugly. Just being able to look it up in a dictionary would be nice, and might also make it easier to add new ones, since we only need to update one place rather than having to worry about keeping intro and the ifs in sync.

But if we don't want to update intro each time we come up with a new generator we still have a problem - it needs to know what to call all the different generators when telling us what our options are. We could put that in the dictionary I mentioned above, or if we're already putting stuff in our generator objects we can add a name field in there as well. The latter is probably tidier.

Taken all together, we might get something like

class MadlibsGenerator:
    def __init__(self, name, descriptions, generator):
        self.name = name
        self.descriptions = descriptions
        self.generator = generator
    
    def __call__(self, *args, **kwargs):
        return self.generator(*args, **kwargs)

def intro(generators):
    print("Welcome to our madlib generator!")
    print("We'll ask you for a serious of words, you'll give us the words, and "
          "we'll make a dope ass story out of it!")
    print("Let's get some info from you first.")
    print("Select the type of madlib you want. ")

    for key in generators:
        print(f"{key}. {generators[key].name}")

    while True:
        selection = input()
        if selection in generators:
            break
        else:
            print(f"No generator \"{selection}\", please try again.")

    return generators[selection]

def user_input(descriptions):
    words = []
    for description in descriptions:
        words.append(input(f"Enter {description}: "))

    return words


def generate_short_story(name, animal, color, item, food):
    return (f"There once was a person named {name}. {name} LOVED "
        f"{animal}s. They were {name}'s favorite animal.\n"
        f"One day, {name} was walking down the street wearing "
        f"{color} pants and a {color} shirt. In fact, all of "
        f"{name}'s\n"
        f"clothes were {color}! In one of {name}'s hand, "
        f"there was a bright green {item}. In the other, there was "
        f"a big {food} that {name} was waiting to eat!")



def generate_poem(adjective, noun1, noun2):
    return (f"Roses are {adjective}\n"
        f"{noun1} are blue\n"
        f"I can't start my day without {noun2}")


if __name__ == "__main__":
    generators = {
        "1": MadlibsGenerator("Short Story",
            ["a name", "an animal", "a color", "an item", "a food"],
            generate_short_story),
        "2": MadlibsGenerator("Poem",
            ["an adjective", "a plural noun", "any noun"],
            generate_poem)
    }

    generator = intro(generators)
    madlib_input = user_input(generator.descriptions)
    print(generator(*madlib_input))

Update

At this point, you might start to think: "Hang on, each of those generator functions just puts strings into another string - creating a function each time we want to do that seems clumsy". And you have a point. While f-strings are nice, there are other options available that let us specify the template in one place and later use it in another. For example, strings already have a format method. We could take advantage of that and replace the generator functions with strings like

poem_template = """
Roses are {0}
{1} are blue
I can't start my day without {2}
"""

And instead of having our generators call these functions, we can have them call .format on these strings like

class MadlibsGenerator:
    def __init__(self, name, descriptions, template):
        self.name = name
        self.descriptions = descriptions
        self.template = template.strip()

    def __call__(self, *args):
        return self.template.format(*args)

And now we can create generators like greeting_generator = MadlibsGenerator("Greeting", ["a name"], "Hi there, {0}!") if we want to. Neat!

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Well this is much cleaner. I figured I'd create a class since I'm trying to make classes out of everything, but learning about when to actually use classes is something I'll need to keep an eye on. I love passing descriptions to user_input and only asking what's required for the type of story. I'm working on understanding the flow of information here from intro to the final print statement, so I might have some more questions later. With regards to using the main block, I'm assuming that now when the file is imported, they can choose to use the functions as needed, is that right? \$\endgroup\$
    – pylearn
    Feb 12, 2021 at 18:23
  • 1
    \$\begingroup\$ @pylearn Honestly, while I think the original class was a bit oddly designed on second thought I don't think using a class here is a bad approach. It really is meaningful to talk about "a generator" (my code example even does that), and avoiding classes did make my code a bit more awkward I feel. I've updated my review (and code) a bit. Also, yes, when someone imports a file they can use any functions, classes and variables from that file however they like, but any code in an if __name__ == "__main__" block doesn't run. \$\endgroup\$
    – Sara J
    Feb 12, 2021 at 19:16
  • \$\begingroup\$ Checking the edit out now (thanks for continuing to answer my questions!). For this code block: def __call__(self, *args, **kwargs): return self.generator(*args, **kwargs) Is it essentially just running a call to the instance of the class with the passed in generator? \$\endgroup\$
    – pylearn
    Feb 12, 2021 at 19:46
  • 1
    \$\begingroup\$ @pylearn Yeah. The __call__ method is just what happens when you call an instance of a class as though it was a function, and in this case it just passes the arguments to the "generator" function that instance has. \$\endgroup\$
    – Sara J
    Feb 12, 2021 at 20:02

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.