7
\$\begingroup\$

This is my Player file. It is mainly for saving purposes. Is it easy to understand and read? Anything I should add or change for improvements?

#Programmer: DeliriousSyntax
#Player.py
#Date: November 22, 2015

import math


class User:
    def __init__(self, user, class_type):
        self.user = user
        self.level = 1
        self.xp = 1
        self.Class = class_type
        self.dex_attr = 0
        self.tech_attr = 0
        self.strength_attr = 0
        self.health_attr = 0
        self.energy_attr = 0
        self.boss_kills = 0
        self.ordinary_kills = 0
        self.towns_visited = 0
        self.money_earned = 0
        self.money = 0
        self.black_jack_wins = 0
        self.beers = 0
        self.houses = 0
        self.rent = 0
        self.shops = 0

    @property
    def drinking_class(self):
        if self.beers <= 25:
            return "Teetotaler"
        elif self.beers <= 250 and self.beers > 25:
            return "Light Drinker"
        elif self.beers > 250:
            return "Alcoholic"

    @property
    def health(self):
        return 110 + (self.health_attr * 4)

    @property
    def energy(self):
        return 50 + (self.energy_attr * 4)

    @property
    def strength(self):
        return 15 + self.strength_attr

    @property
    def tech(self):
        return 15 + self.tech_attr

    @property
    def dex(self):
        return 15 + self.dex_attr

    @property
    def lvl_xp(self):
        return int(math.sqrt(float(self.level) * 999) * self.level)

    def update_level(self):
        while self.xp >= self.lvl_xp:
            self.level+=1
            print("Level Up! \nLevel {} reached! {}xp to next level.".format(lvl, lvl_xp))

    def stats(self):
        return('\n\n==============Player Stats===============\n' + 
            '|-{:<17}-|-{:^17}-|\n'.format(' Name', self.user)   +
            '|-{:<17}-|-{:^17}-|\n'.format(' Class', self.Class) +
            '|-{:<17}-|-{:^17}-|\n'.format(' Level', self.level) +
            '|-{:<17}-|-{:>8}/{:<8}-|\n'.format(' XP', self.xp, self.lvl_xp)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Boss Kills', self.boss_kills)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Normal Kills', self.ordinary_kills)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Towns Visited', self.towns_visited)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Drinking Class', self.drinking_class)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Gambling Wins', self.black_jack_wins)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Houses', self.houses)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Houses Sold', self.houses)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Rent Collected', self.rent)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Shops', self.shops)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Money', self.money)+
            '|-{:<17}-|-{:^17}-|\n'.format(' Total Money', self.money_earned))

##This is for testing 
p = User(' Arse Face', 'Assassin')
def stats():
    Stats = p.stats()
    print(Stats)
\$\endgroup\$
4
  • \$\begingroup\$ nit: 25 < self.beers <= 250 is valid in Python, and you should use it here \$\endgroup\$
    – wchargin
    Nov 26 '15 at 4:28
  • 2
    \$\begingroup\$ If this is for save purposes, it should have a __hash__ method to let you pickle it, or otherwise write it in some serializeable way. Maybe a to_json method? \$\endgroup\$
    – Adam Smith
    Nov 26 '15 at 8:29
  • 2
    \$\begingroup\$ nitpick: the +s on the end of the strings in stats are unnecessary. Strings are automatically concatenated in Python and putting it in parens as you have means you don't even need line continuation \s. \$\endgroup\$
    – Adam Smith
    Nov 26 '15 at 8:31
  • 1
    \$\begingroup\$ @AdamSmith Those two points make for a solid answer, especially as you could explain both of them clearer in an actual post. \$\endgroup\$ Nov 26 '15 at 14:27
3
\$\begingroup\$

Your __init__ is really quite long and you're setting a lot of values to zero, so you can use Python's multiple assignment. This is perfectly valid syntax:

>>> a = b = 1
>>> a
1
>>> b
1

It means that both a and b are instantiated with the same value of 1. You could use this to set a lot of your attributes in one go. Also I find _attr to be a bit of a pointless suffix since it's clear from context these are attributes, and player1.dex_attr actually makes less sense than player1.dex. So I'd remove them:

def __init__(self, user, class_type):
    self.user = user
    self.level = self.xp = 1
    self.Class = class_type
    self.dex = self.tech = self.strength = 0
    self.health = self.energy = 0

I tried to group sensibly, as this suggestion could easily result in an unreadable mess if you're not careful.

Note that if you take mgillett's suggestion and store lists then you wouldn't want to use this method for list or set attributes, as:

    self.towns = self.houses = set()

would both refer to the same set, which is not what you need.

In drinking_class you could make easier logic by using the fact that cases are mutually exclusive. You don't need to check if beers > 25 since if it was you'd have already returned with a string:

@property
def drinking_class(self):
    if self.beers <= 25:
        return "Teetotaler"
    elif self.beers <= 250:
        return "Light Drinker"
    else:
        return "Alcoholic"

You have base stats for your other properties, but it would be better to define these as class constants instead that you refer to in the function.

class User:

    MIN_HEALTH = 110

    ...

    @property
    def health(self):
        return User.MIN_HEALTH + (self.health_attr * 4)

Technically accessing a class attribute could be done with either self or the class name, but using the classname makes it explicit that the individual instances of user have no bearing on MIN_HEALTH.

Lastly in stats, you repeatedly reuse the same string literal. You could just declare it up front to make it clear you're reusing it and shorten lines at the same time:

def stats(self):
    template = '|-{:<17}-|-{:^17}-|\n'
    return('\n\n==============Player Stats===============\n' + 
        template.format(' Name', self.user) +
        template.format(' Class', self.Class) +
        template.format(' Level', self.level) +
        '|-{:<17}-|-{:>8}/{:<8}-|\n'.format(' XP', self.xp, self.lvl_xp)+
        template.format(' Boss Kills', self.boss_kills)+
        template.format(' Normal Kills', self.ordinary_kills)+

This has the added benefit of making it clearer that the xp line is a different format to the others.

\$\endgroup\$
11
\$\begingroup\$

You seem to have a lot of counts of things. towns_visited is a good example. This implies to me that in your game, you would have a variety of towns to be visited. Perhaps, having an empty set for which towns you have visited, and instead of towns_visited, just use a count of that set. Every time you visit a town, add it to the set. The same would apply to houses. Every time you buy a house, add the specific house object to a list and that would take care of houses and what you do with them. I'm assuming you know the difference between a list and a set.

\$\endgroup\$
1
  • \$\begingroup\$ I'll look into doing something like that. Never actually thought about that. Thanks! \$\endgroup\$ Nov 25 '15 at 21:00
7
\$\begingroup\$

You could also consider changing the name of stats() to __str__(), which would enable you to do print(p) directly.

Another considerations in the same method, is to define a few given format patterns beforehand, so they can easily be changed.

All in all the code looks neat and tidy, and I might have written something similar myself. Maybe I would choose self.class_, self.cls or self.player_class instead of self.Class, and possibly using more self._private_variables in due places.

Addendum on private variables

A major point of using classes, is to hide or consolidate information. The public interface of a class (or instantiation of a class) is usually limited versus the internal interface. In your case you have defined most variables and properties to be public (by naming convention like self.health_attr, indicating that all attributes can/should be accessed from both in- and outside of your class.

Usually a better approach would be to hide some of the internal details, and then let methods affecting the object change the internal details. You've made a public property of health, which is good, but it is dependant on health_attr, which to me should be an private variable, like _health_attr. Similarily I would have used energy and _energy_attr.

And then depending on what your _energy_attr really depicts, a method like update_level() could at given points increase the _health_attr or _energy_attr, or alternative if they are more related to actions, you could have attack() reducing the _energy_attr as it uses energy. And if your user was attacked it could affect the _health_attr, and so on.

The core concept is that is kind of strange to have all of your attributes publicly available, instead of some of them being private and affected by class methods. But then again it comes down to the main purpose of your class, and how you build your domain model. To me the class as it stands is very sparse, and missing a lot of methods and interaction for it to be useful, and I would have extended the usage way beyond just a means to save attributes.

\$\endgroup\$
1
  • \$\begingroup\$ Could you explain more about the self._private_variables please. :) \$\endgroup\$ Nov 29 '15 at 18:43
1
\$\begingroup\$

You should add a method for receiving money, so that you can let the class take care of incrementing self.money and self.money_earned. This could be a money property. (Then you would rename self.money to something else.)

You could add docstrings to the class and the properties and document some attributes if you like.

In your testing function: The variable Stats could start with a small letter.

I guess the health and energy properties should be max_health and max_energy.

Why is the class called User and not Player? It seems strange that a class User has a member variable self.user.

All together there is not much to complain about so far. Of course it depends on your use cases whether all attributes make sense.

\$\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.