6
\$\begingroup\$

I bring this code review plea over from suggestions from SO. I've made the suggested edits from that thread, except I kept one sys.exit() call because I was having trouble exiting otherwise. Overall, the code works as desired, and I'm looking for advice on how I can make it even cleaner.

import time
import sys

done = "I'm tired of you. Goodbye."
rainedOut = "Sorry, rain foiled your plans :("
dontUnderstand = "I'm sorry, I don't understand."

def good_weather():
    """Imagine a world where every 5 seconds it rains (good_weather = False),
    then is sunny again (good_weather = True). This function should return
    whether good_weather is True or False at the time it's called.
    """
    return (time.time() % 10) <= 5

def start():
    entries = 0

    while entries < 4:
        choice = raw_input("Hello! What do you want to do right now? Options: 1) Sleep, 2) Work, 3) Enjoy the great outdoors: ")

        if choice == "1":
            print "We are such stuff as dreams are made on, and our little life is rounded with a sleep. - Shakespeare, The Tempest"
        elif choice == "2":
            work()
        elif choice == "3":
            outdoors()
        else:
            print dontUnderstand
            entries += 1
    print done

def work():
    entries = 0
    entries2 = 0

    while entries < 4:
        choice = raw_input("Would you prefer sedentary office work or manual labor in the elements?: ")

        if "office" in choice:
            print "The brain is a wonderful organ; it starts working the moment you get up in the morning and does not stop until you get into the office. -Robert Frost"
            start()
        elif "manual" in choice:
            sunny = good_weather()
            if sunny == True:
                print "A hand that's dirty with honest labor is fit to shake with any neighbor. -Proverb"
                start()
            else:
                if entries2 < 3:
                    print rainedOut
                    entries2 += 1
                    continue #restart loop
                else:
                    print "You should probably just give up now."
                    start()
        else:
            print dontUnderstand
            entries += 1
    print done
    sys.exit()

def outdoors():
    sunny = good_weather()
    if sunny == True:
        print "Adopt the pose of nature; her secret is patience. -Ralph Waldo Emerson"
        start()
    else:
        print rainedOut
        start()

start()
\$\endgroup\$
0

3 Answers 3

4
\$\begingroup\$

Ok here we go.

Style

You should get acquainted with the official python coding style, which is PEP8. I notice especially naming violations with your global variables, which should follow the rules of functions: lowercase with underscore separation.

If-elsing

Perhaps it's just me, but I find heavily usage of if/elif/else messy.

Python does not support the typical switchstatement that could fit here, but there is an idiom that resembles it in python by using a dictionary of functions.

for example:

options = {1 : sleep,
           2 : work,
           3 : outdoors,
}
options[entries]()

this will require you to create an additional sleep function that can be executed to print the text We are such stuff as dreams are made on

This technique however lacks the break system that is available in switch statements, but you don't seem to need that in your code.

Additionally this scales really easily, all you got to do is add a new method, nicely encapsulated, and add it to the dictionary.

Multiple end points

Your code seems to have multiple end points. At the end of your start() function you print done. But also in your work() function you print it and terminate the program manually. It is cleaner to have a smooth main function, with multiple options (may it be function calls), when the function calls return, you simply print done only there and the program stops.

Main function

It is a good habit to use a main function, perhaps even better to rename your start to main and use the following construct:

def main():
    # Your main code

if __name__ == "__main__":
    main()

Goodluck!

\$\endgroup\$
3
\$\begingroup\$

Hello the style of your code seems very good. You should limit your lines to a maximum to 79 as per PEP8. But otherwise you have a very good code style.

Also Python has the following cases for variables, etc.

  • Classes: CammelCase.
  • Constants: UPPER_SNAKE_CASE.
  • Other, mostly: snake_case.

Your code seems to be repetitive in some cases, and very hard coded.

We should first aim to remove repetition, and remove the static code from the dynamic code.

start could become very repetitive very fast. It uses a 'case' statement, and should use a dict. This is good for later when you have more functions like this.

#Python2/3 support.
try:
    input_ = raw_input
except NameError:
    input_ = input

def choice(choice_hint, options, max_trys=4):
    def call():
        entries = 0
        while entries < max_trys:
            choice = raw_input(choice_hint)
            if choice in options:
                if options[choice]():
                    break
            else:
                print(dontUnderstand)
                entries += 1
    return call

If your program didn't use good_weather() then you would only need two functions.
You need to pass functions, not strings. But it's simple.

I removed print done, as you can do that at the end of the program. I added the break, so that you don't have to call sys.exit.

Usage

def delayed_output(output, status=True):
    def call():
        print(output)
        return status
    return call

choice(
    "Hello! What do you want to do right now? Options: 1) Sleep, 2)"
    "Work, 3) Enjoy the great outdoors: ",
    {
        "1": delayed_output(
            "We are such stuff as dreams are made on, and our little life "
            "is rounded with a sleep. - Shakespeare, The Tempest",
            False),
        "2": work,
        "3": outdoors
    })()
print done

work is like start, where outdoors is like sleep.

The changes I would make to outdoors is the removal of start(). You call it on each branch. And you could do nothing and go back to start.
So we will return if we want to break or not.

def function_output(output, function, status=True):
    def call():
        try:
            print(output[function()])
        except KeyError:
            print('Error in `function_output`. Unhandled output.')
        return status
    return call

To improve work, I would use the above functions.

Also True is a singleton. when comparing to singletons use is. And you should not compare to True or False.

# Bad
>>> True == True
True
>>> False != True
True

# Ok for other singletons
>>> True is True
True
>>> False is not True
True

# Good
>>> True
True
>>> not False
True

Also passing the argument entries2 is annoying to implement. You would have to make every function take *args. Something I don't think beginners should know or use.
Also I wouldn't be able to just use the above functions.


To use everything, you would need to change the initial choice.

choice(
    "Hello! What do you want to do right now? Options: 1) Sleep, 2)"
    "Work, 3) Enjoy the great outdoors: ",
    {
        "1": delayed_output(
            "We are such stuff as dreams are made on, and our little life "
            "is rounded with a sleep. - Shakespeare, The Tempest",
            False),
        "2": choice(
            "Would you prefer sedentary office work or manual labor in "
            "the elements?: ",
            {
                "office": delayed_output(
                    "The brain is a wonderful organ; it starts working "
                    "the moment you get up in the morning and does not "
                    "stop until you get into the office. -Robert Frost"
                ),
                "manual": function_output(
                {
                    True: "A hand that's dirty with honest labor is fit "
                          "to shake with any neighbor. -Proverb",
                    False: rainedOut
                },
                good_weather)
            }),
        "3": function_output(
            {
                True: "Adopt the pose of nature; her secret is patience. "
                "-Ralph Waldo Emerson",
                False: rainedOut
            },
            good_weather)
    })()

This way we have no duplicate logic.
We have all the strings close together.
We can remove the global strings, and pass them through the initial function.

We could have the three functions in a local name-space. This would have the advantage of having nothing globally defined.

If you wanted to go a step further, than you could have the input as a JSON object. This would have the advantage of storing all the static information in a different file.

You could start the program like this:

def main():
    with open('my_file.json', 'r') as file_:
        data = json.load(file_)

    # Convert to runnable code, and run it.
    json_to_python_load(data)()

Pros and cons

I changed how your program worked. Quite a lot.

Pros

  • Little to no duplicated logic.
  • Splits static and dynamic code.
  • Very little code.
  • Increased maintainability.
  • Reduce work to add more things.
  • Useable on Python2 and Python3.
    If you dislike it then it is easy to remove.

Cons

  • Not as changeable, we had to remove some of the outdoors logic.
    It wasn't really needed, as you were going to quit the next loop anyway.

  • Can be hard to implement additional features.
    And if you do problems are likely to arise, unless you make it an optional feature.

  • A near re-write of your code.

\$\endgroup\$
0
1
\$\begingroup\$

Diagnosis

You've recognized that sys.exit() is the symptom of a problem — namely, that you are misusing functions as if they were goto labels. Another symptom of this abuse is the excessively deep calls stacks: run this program for a while, then hit Ctrl C to see the stack trace. (If the call stack gets too deep, you'll end up crashing with a stack overflow.)

Suggested solution

@JoeWallis has written an excellent solution. The solution that I am presenting below is a less drastic rewrite that still addresses the root of your problem.

Note that I've renamed start() to main(). "Start" is a name that encourages you to treat it as a goto label.

The key observation is this: the goal of the main loop of the program is to display the main menu, generate a quote, then display the main menu again. Therefore, let's make it the main loop's responsibility to print the quote. The supplementary functions merely return either the quote to be displayed, or None to indicate a fatal exasperation.

import time

RAINED_OUT = "Sorry, rain foiled your plans :("
DONT_UNDERSTAND = "I'm sorry, I don't understand."

def is_sunny():
    """Imagine a world where every 5 seconds it rains (good_weather = False),
    then is sunny again (good_weather = True). This function should return
    whether good_weather is True or False at the time it's called.
    """
    return (time.time() % 10) <= 5

def sleep():
    return "We are such stuff as dreams are made on, and our little life is rounded with a sleep. - Shakespeare, The Tempest"

def work():
    bad_entries = 0
    rainy_days = 0
    while bad_entries < 4:
        choice = raw_input("Would you prefer sedentary office work or manual labor in the elements?: ")
        if "office" in choice:
            return "The brain is a wonderful organ; it starts working the moment you get up in the morning and does not stop until you get into the office. -Robert Frost"
        elif "manual" in choice:
            if is_sunny():
                return "A hand that's dirty with honest labor is fit to shake with any neighbor. -Proverb"
            elif rainy_days < 3:
                print RAINED_OUT
                rainy_days += 1
            else:
                return "You should probably just give up now."
        else:
            print DONT_UNDERSTAND
            bad_entries += 1

def outdoors():
    if is_sunny():
        return "Adopt the pose of nature; her secret is patience. -Ralph Waldo Emerson"
    else:
        return RAINED_OUT

def main():
    submenus = {
        "1": sleep,
        "2": work,
        "3": outdoors,
    }
    bad_entries = 0
    while bad_entries < 4:
        choice = raw_input("Hello! What do you want to do right now? Options: 1) Sleep, 2) Work, 3) Enjoy the great outdoors: ")
        submenu = submenus.get(choice, None)
        if submenu is not None:
            quote = submenu()
            if quote is None:
                break
            print quote
            bad_entries = 0
        else:
            print DONT_UNDERSTAND
            bad_entries += 1
    print "I'm tired of you. Goodbye."

main()

Additional observations

Use ALL_CAPS for constants.

entries and entries2 should have more descriptive names.

It is customary to place the "main" method at the end of the program.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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