7
\$\begingroup\$

I've been doing some self-imposed exercises to get accustomed to Python and below is my code.

import random
from random import randint
from random import randrange
from mainlist import legendaryitems
from mainlist import waystodie
from mainlist import powerup
from mainlist import mood
from mainlist import ponder
import time

class Player(object):
    def __init__(self, name):
        self.name = name
        self.lifecondition = True
        self.hitpoints = 100
        self.items = []
        self.waystodie = []
        self.mood = ""

    def death(self):
        self.lifecondition = False

    def ways_to_die(self, death):
        self.waystodie.append(death)

    def  add_items(self, item):
            self.items.append(item)

    def change_hp(self, hp):
        self.hitpoints = hp

    def change_mood(self, mood):
        self.mood = mood

    def __repr__(self):
        return repr(self.name)





def addplayers(alist):
    players = []
    for item in alist:
        item = Player("%s" % item)
        players.append(item)
        print "%s enters the game with %s HP of life and feels pretty much alive" %  (item.name, item.hitpoints, )
    return players      

def player_mood(playerlist, moodlist):
    for item in playerlist:
        mood = random.choice(moodlist)
        item.change_mood(mood)



playerlist = ["Player1", "Player2", "Player3", "Player4", "Player5", "Player6", "Player7", "Player8", "Player9", "Player10"]


players = addplayers(playerlist)
player_mood(players, mood)

print players[1].mood
print players[2].mood

player_mood(players, mood)

print players[1].mood
print players[2].mood

My intention is to create actions depending on the mood of characters. Basically it's gonna be a "There can be only one!" game. The reason of my post is not so that someone else can write the code for me but for feedback on how to write more clean, efficient code following Python's philosophy at the same time.

I am calling the instances through the players list but is there any other way I should be using to recall them from the class?

I'm able to convey what I want to do into code but is it ok to interact with standalone functions with my class? Or should I be building methods within another Class (utilize inheritance as well) to interact with Player()?

\$\endgroup\$
  • \$\begingroup\$ What is mood in player_mood(players, mood)? \$\endgroup\$ – 200_success Apr 11 '15 at 21:53
  • \$\begingroup\$ @200_success a list. Relax , Fight , Search \$\endgroup\$ – st00pid Apr 11 '15 at 21:57
5
\$\begingroup\$

Note that you can significantly simplify the import statements (two of which you never even use):

import random
import time

from mainlist import legendary_items, mood, ponder, powerup, ways_to_die

I have also arranged and renamed them as recommended by the style guide.


The methods in your class aren't consistently named. For example, ways_to_die and add_items are named differently but do similar things; based on what they actually do I would have named them add_way_to_die and add_item, although as the lists they add to are public attributes you could just let the user append to them without the convenience functions. Similarly, death should probably be named kill - instance methods generally contain verbs, as they do things to the instance.


The change_hp and change_mood methods are more consistently named, but also redundant; why not let the user do e.g. player.mood = 'happy' rather than player.change_mood('happy')? If you need to control e.g. the range of valid values later, you can make it a property:

class Player(object):

    VALID_MOODS = ['happy', 'sad']

    @property
    def mood(self):
        return self._mood  # note "private by convention" leading underscore 

    @mood.setter
    def mood(self, mood):
        if mood not in self.VALID_MOODS:
            raise ValueError('Invalid mood: {}'.format(mood))
        self._mood = mood

playerlist (should be player_list, as above) can be built much more efficiently with a list comprehension, factoring out addplayers entirely:

player_list = [Player("Player{}".format(n+1)) for n in range(10)]

this makes it easier to change the number of players, and harder to accidentally leave one of the numbers out (e.g. [..., 'Player4', 'Player6', ...]) or make a typo (e.g. [..., 'Player4', 'Players5', 'Player6', ...]). If you still want to introduce them, you can easily do so:

for player in player_list:
    print ("{0.name} enters the game with {0.hit_points} HP of life " 
           "and feels pretty much alive").format(player)

Note the use of Python's implicit concatenation of strings to keep the line length under 80 characters, and str.format rather than the more old-fashioned % formatting.


You could also get rid of player_mood (which is not very well-named), for example by:

  1. adding mood as an initialisation parameter (def __init__(self, name, mood):) and calling random.choice in the list comprehension;
  2. adding mood as an optional parameter (def __init__(self, name, mood=None):) and calling random.choice in the __init__ method if mood is None:; or
  3. providing a convenience function Player.set_random_mood to the class (this could be combined with 2.: if mood is None: self.set_random_mood()).

Python indexing is 0-based, so:

print players[1].mood
print players[2].mood

will actually show the mood attributes of 'Player2' and 'Player3'. Not a deal-breaker, but you should be aware of it!


You should generally write docstrings for your modules, classes, methods and functions; this provides useful information to anyone using your code (including you!)

\$\endgroup\$
  • \$\begingroup\$ Hey @jonrsharpe thanks for the feedback. Thanks for the tip about simplifying the import statements that definitely helps. I don't desire user to be able to manipulate the values I d like the mood to be a random and the print commands was more to show/proof/check that the mood is actually changing indeed. -- pressed enter too soon more to add, i have some questions and you wrote some stuff i find very interesting \$\endgroup\$ – st00pid Apr 12 '15 at 15:12
  • \$\begingroup\$ style guide is definitely something I ll read. The problem with setting the VALID_MOODS = ['happy', 'sad'] outside from the init means that all my objects will inherit those values and that is something I don't want to. Can you tell me what '@'property and '@'mood.setter are? python 3? I like your error control at mood.setter function i ll keep that tip for sure. Your comment about playermood though has hit home. I totally get it. Instead of creating standalone functions I will be adding methods within my class to do exactly that. Cheers ^^ \$\endgroup\$ – st00pid Apr 12 '15 at 15:26
  • \$\begingroup\$ VALID_MOODS is a class attribute, shared by all instances; why don't you want that? Properties are available in 2.x and 3.x; see e.g. stackoverflow.com/q/6618002/3001761 \$\endgroup\$ – jonrsharpe Apr 12 '15 at 16:45
  • \$\begingroup\$ there will be no user input , the mood will change between Search, Relax and Fight. Depending on the mood other actions will be triggered. 'VALID_MOODS' like you said would be shared by all objects and since there's no need for user input and error control on that input I see no point to add it. Though would it be structural more wise to have them as predefined attributes and randomly pick them up instead from passing them from a list? \$\endgroup\$ – st00pid Apr 12 '15 at 18:14
  • 1
    \$\begingroup\$ @st00pidthrn then if it's only used for the Player, it should probably be in the same script, at least. It's not clear what your objection to a class attribute is, but you could make it a module-level constant. \$\endgroup\$ – jonrsharpe Apr 12 '15 at 18:37

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.