6
\$\begingroup\$

I'm currently on Ch 4 Lab 4 in Programming Arcade Games with Python and Pygame.

Any feedback on how I could improve the code would be awesome. I'm using Python IDLE 3.4.3.

import random

print("Welcome to Camel!")
print("""You have stolen a camel to make your way across the great Mobi desert.
The natives want their camel back and are chasing you down! Survive your desert trek and out run the natives.""")
print()

#variables
milesTraveled = 0
thirst = 0
camelFatigue = 0
nativesTraveled = -20
canteen = 3
done = False


#start main loop
while not done:
    nativesBehind = milesTraveled - nativesTraveled
    fullSpeed = random.randrange(10, 21)
    moderateSpeed = random.randrange(5, 13)
    print("""
    A. Drink from your canteen.
    B. Ahead at moderate speed.
    C. Ahead full speed.
    D. Stop for the night.
    E. Status check
    Q. Quit.""")
    print()
    userInput = input("Your choice? ")
    if userInput.lower() == "q":
            done = True

#status
    elif userInput.lower() == "e":
        print("Miles traveled: ",milesTraveled)
        print("Drinks in canteen: ",canteen)
        print("Your camel has ",camelFatigue,"amount of fatigue.")
        print("The natives are ",nativesBehind,"miles behind you.")
#stop for night
    elif userInput.lower() == "d":
        camelFatigue *= 0
        print("Your camel feels refreshed and happy his fatigue is now ",camelFatigue)
        nativesTraveled += random.randrange(7, 15)
#move full speed
    elif userInput.lower() == "c":
        print("You traveled ",fullSpeed,"miles!")
        milesTraveled += fullSpeed
        thirst += 1
        camelFatigue += random.randrange(1, 4)
        nativesTraveled += random.randrange(7, 15)
        oasis = random.randrange(1, 21)

#move moderate speed
    elif userInput.lower() == "b":
        print("You traveled ",moderateSpeed,"miles!")
        milesTraveled += moderateSpeed
        thirst += 1
        camelFatigue += 1
        nativesTraveled += random.randrange(7, 15)
        oasis = random.randrange(1, 21)

        #drink canteen
    elif userInput.lower() == "a":
        if canteen == 0:
            print("You're out of water.")
        else:
            canteen -= 1
            thirst *= 0
            print("You have ",canteen,"drinks left and you are no longer thirsty.")

#not done check
    if oasis == 20:
        camelFatigue *= 0
        thirst *= 0
        canteen = 3
        print("You found an oasis! After taking a drink you filled your canteen and the camel is refreshed.")
    if nativesBehind <= 15:
        print("The natives are drawing near!")
    if milesTraveled >= 200 and not done:
        print("You made it across the desert, you win!")
        done = True
    if nativesTraveled >= milesTraveled:
        print("The natives caught and beheaded you.")
        print("You're dead!")
        done = True
    if thirst > 4 and thirst <= 6 and not done:
        print("You are thirsty")
    if thirst > 6:
        print("You died of dehydration!")
        done = True
    if camelFatigue > 5 and camelFatigue <= 8 and not done:
        print("Your camel is getting tired.")
    if camelFatigue > 8:
        print("Your camel is dead.")
        done = True
\$\endgroup\$
3
  • 1
    \$\begingroup\$ @justsomenoob2020 Minor changes, like 'oops, the program won't run' *cough* import random *cough* are fine. As you went 'I need to fix all these random.randranges' it could have been an answer and you would get a badge. - Just to note, you may want to fix import random \$\endgroup\$
    – Peilonrayz
    Oct 4, 2015 at 20:52
  • \$\begingroup\$ i think that happened from ctrl-k not sure. its not indented like that in my original file. the game works fine its just that its too easy. i just wanted input on how to improve the code (make shorter, diff commands i could have used etc) \$\endgroup\$ Oct 5, 2015 at 2:50
  • 1
    \$\begingroup\$ Editing the question was okay as there weren't any answers yet. But it's still best not to put "EDIT" markers in the question and mention what was edited. None of that is essential to know and should just stay in the actual edit summary. \$\endgroup\$
    – Jamal
    Oct 5, 2015 at 6:21

4 Answers 4

4
\$\begingroup\$

Bug

I ran your game and here is the output:

$ python3 test.py
Welcome to Camel!
You have stolen a camel to make your way across the great Mobi desert.
The natives want their camel back and are chasing you down! Survive your desert trek and out run the natives.


    A. Drink from your canteen.
    B. Ahead at moderate speed.
    C. Ahead full speed.
    D. Stop for the night.
    E. Status check
    Q. Quit.

Your choice? D
Your camel feels refreshed and happy his fatigue is now  0
Traceback (most recent call last):
  File "test.py", line 73, in <module>
    if oasis == 20:
NameError: name 'oasis' is not defined

If I select any option other than C and D, I get the NameError. You can fix it by adding

oasis = 0

before the while loop.

Suggestions

  1. This

    print("Welcome to Camel!")
    print("""You have stolen a camel to make your way across the great Mobi desert.
    The natives want their camel back and are chasing you down! Survive your desert trek and out run the natives.""")
    print()
    

    can be written as

    print("Welcome to Camel!")
    print("""You have stolen a camel to make your way across the great Mobi desert.
    The natives want their camel back and are chasing you down! Survive your desert trek and out run the natives.\n""")
    

    Similarly,

    print("""
    A. Drink from your canteen.
    B. Ahead at moderate speed.
    C. Ahead full speed.
    D. Stop for the night.
    E. Status check
    Q. Quit.""")
    print()
    

    can be written as

    print("""
    A. Drink from your canteen.
    B. Ahead at moderate speed.
    C. Ahead full speed.
    D. Stop for the night.
    E. Status check
    Q. Quit.\n""")
    

    thereby eliminating the need for a seperate print().

  2. You can eliminate the need for a Done variable by using an infinite loop

    while True:
    

    and replacing every Done = True with break. This will also eliminate the need for using and not done in some of the conditions.

  3. Instead of doing stuff like

    camelFatigue *= 0
    thirst *= 0
    

    You can use

    camelFatigue = thirst = 0
    
  4. You have

    if userInput.lower() == "q":
        # ...
    elif userInput.lower() == "e":
        # ...
    elif userInput.lower() == "d":
        # ...
    elif userInput.lower() == "c":
        # ...
    elif userInput.lower() == "b":
        # ...
    elif userInput.lower() == "a":
        # ...
    

    You should also have an else printing an error message in case the user enters something invalid. Like so:

    if userInput.lower() == "q":
        # ...
    elif userInput.lower() == "e":
        # ...
    elif userInput.lower() == "d":
        # ...
    elif userInput.lower() == "c":
        # ...
    elif userInput.lower() == "b":
        # ...
    elif userInput.lower() == "a":
        # ...
    else:
        print("Invalid input; Try again") # Print error message
    
  5. You can also print how much thirsty you are when the user selects "E"(status check)
\$\endgroup\$
1
  • \$\begingroup\$ You can even put the closing """ on a separate line, avoiding the \n. \$\endgroup\$
    – mkrieger1
    Oct 5, 2015 at 21:00
5
\$\begingroup\$

Bad instructions

Perhaps the lab is instructing you specifically to write code in a certain way just for learning purposes, but there is some questionable advice in the instructions that I would recommend disregarding.

Printing the game instructions using multiple print statements is poor practice. Combining the instructions into one long triple-quoted string is more readable and efficient.

Introducing a done flag variable is a bad idea in general. If your goal is to direct the execution of code in a loop, use statements like continue, break, return, or raise. Setting a variable just to have it tested later is needlessly indirect. In fact, I would say that for this exercise, a done flag fails to capture enough state information for a realistic game. (More on that below.)

Logic

The input is case-insensitive. To avoid code repetition, you should immediately normalize the input to lowercase (or uppercase, as the instructions suggest):

user_choice = input("Your choice? ").upper()

There are five menu options, plus the "Quit" option. Of those five, two are "instantaneous", and three involve the passage of time. Of the three options that involve passage of time, two cause you to cover some distance. The code should therefore be structured such that similar code is placed together. Also, you should avoid generating and discarding random numbers that you aren't going to use (for example, fullSpeed = random.randrange(10, 21) when the user hasn't even decided to make a move).

# Quit
if user_choice == "Q":
    break

# Status check
elif user_choice == "E":
    print(…)

# Drink from canteen
elif user_choice == "A":
    if canteen == 0:
        print("You're out of water.")
    else:
        canteen -= 1
        thirst = 0
        print(…)

# Stop for the night
elif user_choice == "D":
    natives_traveled += random.randrange(7, 15)
    camel_fatigue = 0
    print(…)

# Moderate speed
elif user_choice == "B":
    natives_traveled += random.randrange(7, 15)
    miles_traveled += random.randrange(5, 13)
    camel_fatigue += 1
    thirst += 1

# Full speed
elif user_choice == "C":
    natives_traveled += random.randrange(7, 15)
    miles_traveled += random.randrange(10, 21)
    camel_fatigue += random.randrange(1, 4)
    thirst += 1

It's odd how you write variable *= 0 when you could just set variable = 0.

By my interpretation, it should not be possible to stumble across an oasis while resting for the night. It would be even more realistic (and easier to code) if the probability of finding an oasis were proportional to the distance covered, but I don't get to make the rules.

When evaluating the situation at the end of the loop, I would place the position checks first, then the health checks, then any applicable warnings. You don't really care about the natives drawing near or finding an oasis if you've already crossed the finish line. You can also simplify the thirst and fatigue checks if you test for death first. (By the way, in Python, you can also write double-ended inequalities like 5 < camelFatigue <= 8.)

# Position checks
if miles_traveled >= 200:
    print("You made it across the desert!")
    break
elif natives_traveled >= miles_traveled:
    print("The natives caught and beheaded you.\nYou're dead!")
    break

# Health checks
# 1 in 20 chance of finding an oasis
if user_choice != "D" and random.randrange(20) == 0:
    camel_fatigue, thirst, canteen = 0, 0, 3
    print("You have found…")
elif thirst > 6:
    print("You died of dehydration!")
    break
elif camel_fatigue > 8:
    print("Your camel is dead.")
    break

# Warnings
if natives_traveled >= miles_traveled - 15:
    print("The natives are drawing near!")
if thirst > 4:
    print("You are thirsty")
if camel_fatigue > 5:
    print("Your camel is getting tired.")
\$\endgroup\$
3
\$\begingroup\$

Style reviews

I would suggest to read up on PEP8, as it quite common to follow this when programming in Python I've understood. Here are some comments to your coding style based on these guidelines:

  • Variable names should be snake_case – That is camel_fatigue, not camelFatigue
  • Avoid larger code blocks, use functions instead – This has two large consequences for your code. First of all I would suggest that if your starts spanning several pages, you should seriously consider start using functions, and secondly start using the following code as a wrapper around your main code:
def main():
    # Your code goes here!!

if __name__ == '__main__':
    main()
  • Let comments stay at same indentation level as code – The way you've written the comments before all the if and elif blocks, they kind of look like functions, and in that respect they disturb the read of the code flow. Keep comments at same indentation level, and in this case you could make them into functions
  • Consider using classes to consolidate code – Your code is repeatedly changing the game variables, and these are natural to put into a class
  • Avoid repetitive if/elif blocks – These can often be refactored into nicer code. For example you could have a dictionary of functions to call, where the userInput is the key
  • Don't repeat code like userInput.lower() – In all your tests you use the lower case version, and that is fine. But why don't you do this operation once and for all before starting to test for equality?
  • Fix the bug related to oasis – If you start by moving, the oasis is not set yet. One suggestion could be to set it at start, and each time you have passed the previous oasis
  • Avoid flag variables like done – It is better to use while true: in the combination with break. And if you have to have it, please use something clearer to your context, indicating what is done or what has happened
  • Avoid magic numbers – Having magic numbers like the levels of fatigue or thirst or distance across the desert, is usually not a good idea. When it comes to the amount you have in your code, I would either opt for a specific dict with all of them, or to read them from a config file. In my code, I read them from a config file, which has the added benefit of 'hiding' the values from guys playing the game after looking at the logic. :-)

Code refactor

Disclaimer: I got carried away when refactoring your code, so you might not recognise some of it. The three major changes are using a class to encapsulate game logic, using a config file to avoid all the magic numbers and using dictionaries to keep lists of actions (both for main menu and for game endings)

Feel free to ignore this part of my answer, but here is my heavily refactored code:

import random
from collections import OrderedDict
import ConfigParser           # configparser in Python 3

MENU_INDENTATION = '    '

def main():
    game = CamelGame()

    game_actions = OrderedDict()
    game_actions['a'] = ('Drink from your canteen', game.drink)
    game_actions['b'] = ('Ahead at moderate speed', game.ahead_moderate_speed)
    game_actions['c'] = ('Ahead at full speed', game.ahead_full_speed)
    game_actions['d'] = ('Stop for the night', game.sleep)
    game_actions['e'] = ('Status check', game.check_status)
    game_actions['q'] = ('Quit', game.end_game)

    #start main loop
    while game.continues():
        game.start_turn()

        for action_key in game_actions:
            txt, _ = game_actions[action_key]
            print('{}{}. {}'.format(MENU_INDENTATION, action_key, txt))

        chosen_action = ''
        while chosen_action not in game_actions:
            # In Python3 use input (I think)
            chosen_action =  raw_input("Your choice? ").lower()

            if chosen_action in game_actions:
                game_actions[chosen_action][1]()
            else:
                print "Not a valid choice!"

        game.end_turn()


class CamelGame():

    def __init__(self):
        #variables

        self._messages = []
        self._ongoing_game = True
        self._read_config()

        # If reading of config fails, it will self terminate the game,
        # and in this case, there is no point in continueing initialisation
        if not self._ongoing_game:
            print('One or configuration parameters are missing or wrong')
            self._print_messages()
            print('Will terminate game!')
            return

        self.miles_traveled = 0
        self.thirst = 0
        self.camel_fatigue = 0
        self.natives_traveled = self.config['start_natives']
        self.canteen = self.config['start_canteen']
        self.oasis = self._random_range('oasis_distance')
        self._add_message(
                'Welcome to a new Camel game!\n\n'
                'You have stolen a camel to make your way across the great \n'
                'Mobi desert. The natives want their camel back and are chasing \n'
                'you down! Survive your desert trek and out run the natives.\n')


        self._print_messages()

    def _read_config(self):
        """Reads a lot of magic number from config file"""

        config_section = self.__class__.__name__
        config_parser = ConfigParser.ConfigParser()
        config_parser.read('./{}.cfg'.format(config_section))

        self.config = {}
        for option in ['camel_fatigue_min', 'camel_fatigue_max',
                       'full_speed_min', 'full_speed_max',
                       'mod_speed_min', 'mod_speed_max',
                       'oasis_distance_min', 'oasis_distance_max',
                       'native_travel_min', 'native_travel_max',
                       'start_natives',
                       'start_canteen',
                       'miles_across_desert',
                       'natives_close',
                       'camel_warning',
                       'camel_death',
                       'thirst_warning',
                       'thirst_death' ]:
            try:
                self.config[option] = config_parser.getint(config_section, option)
            except ValueError:
                self._add_message('  Config option \'{}\' has wrong format: {}'.format(
                                  option, config_parser.get(config_section, option)))
                self._ongoing_game = False
            except ConfigParser.NoOptionError:
                self._add_message('  Config option \'{}\' is missing'.format(option))
                self._ongoing_game = False




    def _random_range(self, config_prefix):
        """Returns a random range based on a range from self.config"""
        return random.randrange(self.config['{}_min'.format(config_prefix)],
                                self.config['{}_max'.format(config_prefix)])

    def _add_message(self, status):
        """Accumulate status information"""
        self._messages.append(status)


    def _print_messages(self):
        """Print the accumulated status of the game"""
        if len(self._messages) == 0:
            return

        print('\n'.join(self._messages))
        self._messages = []

        print # To get some space in output


    def start_turn(self):
        """Do common start of turn stuff."""
        self.natives_behind = self.miles_traveled - self.natives_traveled
        self.full_speed = self._random_range('full_speed')
        self.moderate_speed = self._random_range('mod_speed')

    def check_status(self):
        """Do a status check, and print it"""
        self._add_message('Miles traveled: {}'.format(self.miles_traveled))
        self._add_message('Drinks left in canteen: {}'.format(self.canteen))
        self._add_message('Your camel has {} amount of fatigue'.format(self.camel_fatigue))
        self._add_message('The natives are {} miles behind you'.format(self.natives_behind))

        self._print_messages()


    def sleep(self):
        """Stop for night, and let camels restore strength. Change natives speed"""
        self.camel_fatigue = 0
        self._add_message('Your camel feels refreshed')
        self._move_natives()

    def _move_natives(self):
        """Move natives a random amount forward"""
        self.natives_traveled += self._random_range('native_travel')


    def ahead_full_speed(self):
        """Move ahead at full speed"""
        self._add_message('You traveled {} miles!'.format(self.full_speed))
        self.miles_traveled += self.full_speed
        self.thirst += 1
        self.camel_fatigue += self._random_range('camel_fatigue')
        self._move_natives()


    def ahead_moderate_speed(self):
        """Move ahead at moderate speed"""
        self._add_message('You traveled {} miles!'.format(self.moderate_speed))
        self.miles_traveled += self.moderate_speed
        self.thirst += 1
        self.camel_fatigue += self.config['camel_fatigue_min']
        self._move_natives()


    def drink(self):
        """Drink from canteen."""
        if self.canteen == 0:
            self._add_message('You\'re out of water.')
        else:
            self.canteen -= 1
            self.thirst = 0
            self._add_message('You have {} drinks left and '
                              'you are no longer thirsty.'.format(self.canteen))


    def continues(self):
        """Check overall game status"""
        return self._ongoing_game


    def _has_crossed_desert(self):
        return self.miles_traveled >= self.config['miles_across_desert']


    def _has_been_beheaded(self):
        return self.natives_traveled >= self.miles_traveled


    def _has_died_of_thirst(self):
        return self.thirst > self.config['thirst_death']


    def _has_killed_camel(self):
        return self.camel_fatigue > self.config['camel_death']

    def end_game(self):
        """End the game prematurely"""
        self._add_message('You turn yourself in to the natives!')
        self._add_message('As a special favour, the natives do not behead you!')
        self._ongoing_game = False
        self._print_messages()


    def end_turn(self):
        """Check overall game status"""

        if not self._ongoing_game:
            return

        # Automatic end of turn actions
        if self.miles_traveled == self.oasis:
            self.camel_fatigue = 0
            self.thirst = 0
            self.canteen = self.config['start_canteen']
            self._add_message('You found an oasis!\n'
                                'After taking a drink you filled your canteen\n'
                                'and the camel is refreshed.')

        # Game ending conditions
        self.game_endings = [
            ( self._has_crossed_desert, 'You made it across the desert, you win!'),
            ( self._has_been_beheaded, 'The natives caught and beheaded you.\nYou\'re dead!'),
            ( self._has_died_of_thirst, 'You died of dehydration!'),
            ( self._has_killed_camel, 'Your camel is dead!'),
        ]

        for (end_condition, end_message) in self.game_endings:
            if end_condition():
                self._add_message(end_message)
                self._ongoing_game = False
                self._print_messages()
                return

        # Add warnings
        if (self.camel_fatigue > self.config['camel_warning']
            and self.camel_fatigue <= self.config['camel_death']):

            self._add_message('Your camel is getting tired.')

        if self.natives_behind <= self.config['natives_close']:
            self._add_message('The natives are drawing near!')

        if (self.thirst > self.config['thirst_warning']
           and self.thirst <= self.config['thirst_death']):
            self._add_message('You are thirsty.')

        self._print_messages()


if __name__ == '__main__':
    main()

And here is the needed config file (named after the class, CamelGame.cfg:

[CamelGame]
# camel_fatigue - used for determining amount of fatigue for camel running
# at full speed. At moderate speed game uses the min value
camel_fatigue_min = 1
camel_fatigue_max = 4
# full_speed - used for determining full speed distance
full_speed_min = 10
full_speed_max = 21
# mod_speed - used for determining moderate speed distance
mod_speed_min = 5
mod_speed_max = 13
# and so on ...
oasis_distance_min = 1
oasis_distance_max = 21
native_travel_min = 7
native_travel_max = 15
start_natives  = -20
start_canteen = 3
miles_across_desert = 200
natives_close = 15
camel_warning = 5
camel_death = 8
thirst_warning = 4
thirst_death = 6

Do note that config file is somewhat picky and needs for each line start with either a configuration option, section divider([) or comment starter (#).

Some additional notes regarding the refactored code:

  • I've left the actual main menu outside of the class, as this could be changed in a later edition to other mechanisms driving the game. I.e. a curses driven menu where the menu is fixed and you keep on pressing keys, or a graphical version with button widgets for the actions
  • Instead of directly printing, I've chosen to use an internal table, self._messages, to add up messages. This allows for a later edition to print all messages differently, i.e. to a web page or text widget
  • With the introduction of a class, the function declaration and referencing of variables within the class has changed a little. Class methods need to have a first parameter of self, and all internal variables to the class needs to be prefixed with this, i.e. self.miles_traveled. Variables not intended for use outside of the class has a prefix of underscore, i.e. self._ongoing_game
  • All the conditions to end the game has been made into predicate methods, and listed in game_endings. This allows for a looping of this condition, and a break out if one end game conditions has occured. Same could have been applied to the warnings, but just for fun I used both methods to allow for you to compare them up against each other.
  • Have added docstrings to most methods to help understand what the methods does in the context of the game

Coding is supposed to be fun, and there are tons of lessons to be learned for us all out there. Hope this somewhate elaborate code example can give you some ideas for further coding! Happy coding!

\$\endgroup\$
2
\$\begingroup\$

Use functions

One big issue I see is that you use no functions at all. In case you don't know, functions are ways of storing blocks of code so that you can easily call on them from multiple places. It's good practice to split code into functions when it's repeating essentially the same behaviour more than once. For example, the natives advancing seems to always be the same, so that's a prime candidate.

You could define it like this:

def natives_advance():
    return random.randrange(7, 15)

And then call it like this:

natives_traveled += natives_advance()

The big advantage is that you could change the values you're passing to randrange and they would update everywhere. But you could also modify them contextually too. For example, maybe you want them to be able to move further based on how far away from the player they are, then you could modify the function definition to this:

def natives_advance(distance):
    return random.randrange(7, 15 + distance)

And then if you call it like this:

natives_traveled += natives_advance(natives_behind)

You can see how you're dynamically adjusting the function now. Try to approach a lot of how your game works in this way, it makes it far easier to make changes and it makes for more readable code when there's discrete chunks of functionality.

Minor style notes

Instead of using print() you can use the newline character \n to print an empty line. But in this case, you're using a multiline string anyway. so just have an empty line at the end.

print("""You have stolen a camel to make your way across the great Mobi desert.
The natives want their camel back and are chasing you down! Survive your desert trek and out run the natives.
""")

#variables is an unnecessary comment, anyone who knows Python can tell what these are. Use comments to explain the context of more abstract reasons for things, for example you could say

# nativesTraveled stores progress of the natives you're running from

On that note, you should use snake_case for naming variables and functions in Python. It's the accepted style in the style guide PEP0008, which is essential reading. Applied to you variables, that'd mean you have

miles_traveled = 0
thirst = 0
camel_fatigue = 0
natives_traveled = -20
canteen = 3
done = False

When the user inputs q to quit it's still a good idea to print an acceptance of that input, like print("Quitting game..."). The user may have entered the letter accidentally or somehow misunderstood, so giving no feedback other than ending the script could be jarring.

You should use str.format when trying to print variables. It will coerce things to strings and makes it easy to write lines like this:

    print("Your camel has ",camelFatigue,"amount of fatigue.")

which could instead be

    print("Your camel has {} amount of fatigue.".format(camel_fatigue))
\$\endgroup\$

Your Answer

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

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