Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am in the process of creating a text-based RPG for the purpose of learning more about OOP in Python. Right now things are going well, but I'm facing a problem that I don't exactly can find a good answer: Skill implementation. I also think that I'm going to face similar problem with the Items.

At this moment I have 3 classes(professions): Fighter, Rogue, and Mage. Each of these classes should have their own skills, but, skills have similar things(name, description, MP usage), so I created a BasicSkill class, and 3 subclasses: PassiveSkill, ActiveSkill and ChanelledSkill.

PassiveSkill are skills that doesn't require MP to use, are always active, and in most of cases, give the player buffs, like dual-wielding, more resistance, etc. ActiveSkill are skills that require MP to be used, and, in majority, cause damage or a instant buff. Finally, ChanelledSkill are skills that require more than one turn to be used, cost more MP than the others, and can be interrupted

But, when comes to the skill creation, I can only think in something like this:

def hack_and_slash(self):
        """This is a Fighter skill, it's melee and has no cool down."""
        from skills import ActiveSkill
        hack_and_slash = ActiveSkill("Hack n' Slash", "A powerful double attack that has a chance of causing bleeding.",
                                     8, 3)
        self.mp -= hack_and_slash.
        # more about the skill goes here, like what it does in terms of damaging, buffing, etc.

Which I think is not exactly what I'm aiming for, since I have to instantiate it every time I use, or something in the same lines.

I want to create the skills in a way that would require less instantiation, and also make more possible to cast. Also, all characters(player, npc, and enemies) can cast skills.

There's a bunch of files in this project, so I will post the ones I think are more relevant here, and, if you want, you can check out the project here.

This is my Player class:

from character import Character


# TODO implementation of player basic attacks and abilities
# TODO implementation of a D&D style skill system(diplomacy, stealth, streetwise, etc)

class Player(Character):
    LEVEL_UP = 100  # initial XP to lvl up
    COMMANDS = {'attack': 'a', 'character': 'c', 'inventory': 'i'}  # commands available for the player
    # in future this skills will be used inside and outside combat situations, for like take out an enemy in stealth,
    # or convince a guard to let you in a closed door
    SKILLS = {'persuasion': 0, 'intimidation': 0, 'stealth': 0, 'perception': 0}
    DAILY_SELF_HEALS = 4  # each ingame day player can use 4 self heals, it resets after getting the rested status
    ATTRIBUTES = {'strenght': 0, 'constitution': 0, 'dexterity': 0, 'intelligence': 0, 'wisdom': 0, 'charisma': 0}

    def __init__(self, armor_class):
        super().__init__(input("Tell us your name, hero:\n>"), 20, 10, {'gold': 10, 'torch': 1}, armor_class, 1)
        self.exp = 0
        self.max_hp = self.hp  # max HP of the player
        self.max_mp = self.mp  # max MP of the player
        self._strength = 0  # should I change it to dictionary with the attributes instead?
        self._constitution = 0
        self._dexterity = 0
        self._intelligence = 0
        self._wisdom = 0
        self._charisma = 0

    def level_up(self):
        if self.exp >= self.LEVEL_UP:
            self.lvl += 1
            self.LEVEL_UP *= 1.25
            self.LEVEL_UP = int(self.LEVEL_UP)
            self.exp = 0
            stat_choice = input(
                "You have 2 points to spend in your attributes!\nType which attributes you want to raise up: ")
            stat_choice = stat_choice.split(',')
            self.ATTRIBUTES[stat_choice[0]] += 1
            self.ATTRIBUTES[stat_choice[1]] += 1
            self.max_hp = int(self.max_hp * 1.1)
            self.max_mp = int(self.max_mp * 1.1)
            return True
        else:
            return False

    def gain_exp(self, exp):
        self.exp += exp
        print("You gained %d XP" % exp)
        if self.level_up():
            print("Congratulations, you gained a level!\nYour current level is %d\n" % self.lvl)
        else:
            print("Your current XP is %d/%d\n" % (self.exp, self.LEVEL_UP))

    def get_loot(self, enemy):
        # buggy right now, problem with remove inventory from dead enemy
        for item in enemy.inventory:
            while True:
                get_item = input("You found %s! do you want to get it?(Y=yes/N=no/A=all items)" % item)
                if get_item.lower() == "a":
                    self.inventory.update(enemy.inventory)
                    enemy.inventory = {}
                    return
                elif get_item.lower() == "y":
                    self.inventory[item] = enemy.inventory[item]
                    enemy.inventory.pop(item, None)
                    break
                elif get_item.lower() == "n":
                    break
                else:
                    print("Unfortunately, you don't have this choice hero, take a look again...")

    def combat(self, enemy):
        """
        the combat between two entities(player and enemy)
        :param enemy: the enemy of current character
        :return:
        """
        if super().combat(enemy):
            self.gain_exp(10)
            self.get_loot(enemy)
        else:
            print("\t\t[GAME OVER...]\t\t")

    def self_heal(self):
        if self.DAILY_SELF_HEALS > 0:
            self.DAILY_SELF_HEALS -= 1
            self.hp += int(self.max_hp / 4)

    def rest(self):
        rest_time = input("\tHow much time would you like to rest? ")
        if rest_time >= 6:
            self.STATUS['rested'] = True

    def __str__(self):
        str_info = "\tName: [%s]\tLEVEL: %d\n\tHP: %2d\t\tMP: %2d" % (self.name, self.lvl, self.hp, self.mp)
        str_stats = "\t\tSTR: %2d\n\t\tCON: %2d\n\t\tDEX: %2d\n\t\tINT: %2d\n\t\tWIS: %2d\n\t\tCHA: %2d\n" % (
            self._strength, self._constitution, self._dexterity, self._intelligence, self._wisdom,
            self._charisma)
        return "|%s|\n%s" % (str_info, str_stats)

This, my Character class:

# all character, being Player, NPC, or Enemy content should be implemented here
from utils import Dice


    class Character:
        """
        Main character class, all living entities derivate from it
        """

        # a dictionary of possible status of player, the idea is that this status change gameplay/combat in the future.
        # By default all characters are rested.
        STATUS = {'rested': True, 'hunger': False, 'poisoned': False, 'bleeding': False, 'blind': False, 'frozen': False,
                  'paralyzed': False, 'dead': False}
        # Right now there are only four equipment slot, which one should receive an armor object, not yet implemented
        EQUIPMENT_SLOTS = {'head': None, 'chest': None, 'legs': None, 'boots': None}

        def __init__(self, name, hp, mp, inventory, armor_class, lvl):
            """
            Constructor for a character
            :param name: name for the character
            :type name: str
            :param hp: hit points(health) for the character
            :type hp: int
            :param mp: magic points(mana) for the character
            :type mp: int
            :param inventory: inventory of character(gold, items, equips)
            :type inventory: dict
            :param armor_class: main defense
            :type armor_class: int
            :param lvl: level of character
            :type lvl: int
            :return: Character object
            """
            self.name = name
            self.hp = hp
            self.mp = mp
            self.inventory = inventory
            self.armor_class = armor_class
            self.lvl = lvl
            self.atk_bonus = 1
            self.atk_dmg = 4
            self.movement_speed = 6

        def take_dmg(self, dmg):
            self.hp -= dmg
            if self.hp <= 0:
                print("\n[%s has died!]" % self.name)
                self.STATUS['dead'] = True
            else:
                print("\n[%s has %d health left" % (self.name, self.hp))

        def attack(self, enemy):
            """
            All characters can attack, so this is the main attack(physical) method
            :param enemy: Enemy that is in combat with character
            :return:
            """
            d20 = Dice(20)
            dmg = self.atk_dmg
            if d20.roll() + self.atk_bonus >= enemy.armor_class:
                if d20 == 20:
                    print("CRITICAL HIT!")
                    dmg *= 2
                else:
                    dice_dmg = Dice(self.atk_dmg)
                    dmg = dice_dmg.roll()
                print("\n[%s hit %s for %d damage]" % (self.name, enemy.name, dmg))
                enemy.take_dmg(dmg)
                if enemy.STATUS['dead']:
                    return
            else:
                print("\n[%s missed]" % self.name)

        def combat(self, enemy):
            while True:
                self.attack(enemy)
                if enemy.STATUS['dead']:
                    return True
                enemy.attack(self)
                if self.STATUS['dead']:
                    break
            return False

        def equip_armor(self, armor_piece):
            """
            Equip a piece of armor in the correspondent slot
            :param armor_piece: armor piece to equip
            :type armor_piece: object
            :return:
            """
            replace_equip = input(
                "Would you like to replace %s with %s?(Y/N)" % (self.EQUIPMENT_SLOTS['head'], armor_piece.name))
            if replace_equip.lower() == "y":
                self.EQUIPMENT_SLOTS['head'] = armor_piece
                return
            else:
                return

        def list_inventory(self):
            """
            list a character's inventory
            :return:
            """
            print("\t\t[%s] INVENTORY:\t\t\n" % self.name)
            for item in self.inventory:
                print("%s:\t%d" % (item, self.inventory[item]))

The Skill class:

# All basic skill implementation should be here


    class BasicSkill:
        SKILL_TYPE = {'passive': False, 'active': False, 'channeled': False}

        def __init__(self, name, description, skill_type, skill_dmg=0, skill_range=0):
            """
            Basic Skill class
            :param name: skill name
            :type name: string
            :param description: description
            :type description: string
            :param skill_type: passive, active or channeled
            :type skill_type: string
            :param skill_dmg: default=0, meaning it's passive
            :type skill_dmg: int
            :param skill_range: default=0, meaning it's personal
            :type skill_range: int
            :return:
            """
            self._name = name
            self._description = description
            self.SKILL_TYPE[skill_type] = True
            self.skill_dmg = skill_dmg
            self.skill_range = skill_range


    class PassiveSkill(BasicSkill):
        def __init__(self, name, description, skill_type='passive'):
            super().__init__(name, description, skill_type)


    class ActiveSkill(BasicSkill):
        def __init__(self, name, description, skill_dmg, mp_use, cooldown=1, skill_type='active', skill_range=1):
            super().__init__(name, description, skill_type, skill_dmg, skill_range)
            self.mp_use = mp_use
            self.cooldown = cooldown


    class ChanneledSkill(BasicSkill):
        def __init__(self, name, description, skill_dmg, channeling_time, cooldown=2, skill_type='chanelled',
                     skill_range=1):
            super().__init__(name, description, skill_type, skill_dmg, skill_range)
            self.channeling_time = channeling_time
            self.cooldown = cooldown

As an example, the Fighter class (all the classes/professions are implemented in similar way):

from player import Player


# TODO implement fighter stats and attacks

class Fighter(Player):
    """
    Fighter class, a strong warrior that uses meele attacks to face his opponents
    """

    def __init__(self):
        super().__init__(armor_class=10)
        print("\nYou have chosen the path of strength!")
        self._strength += 2
        self._constitution += 2
        self.atk_bonus += 1
        self.atk_dmg += 3

    def hack_and_slash(self):
        from skills import ActiveSkill
        hack_and_slash = ActiveSkill("Hack n' Slash", "A powerful double attack that has a chance of causing bleeding.",
                                     8, 3)
        self.mp -= hack_and_slash.mp_use

    def __str__(self):
        super().__str__()

Any ideas are welcome, as well as code improvement tips. I tried to figure out a good way to make this skill implementation system, but I couldn't exactly find one. The only alternative I tough was a dictionary of skills, but I don't know how would it work.

I'm using python 3, in a Ubuntu 14.04LTS, using PyCharm as my IDE.

share|improve this question
    
Any specific python version, 3, 2.7? – Pimgd Feb 25 at 14:30
    
I'm currently using Python 3, I'll update the post with this info – inblank Feb 25 at 14:32
up vote 7 down vote accepted

Aside from Pimgd's comments, I'd like to add that from an architecture point of view, you might want to reconsider your abilities. To make them easier to maintain, you should design all (or nearly all) abilities as just a dictionary of pure data. Figure out all the possible interactions and make key-value pairs for them.

For example:

{ 
  'name': 'Hack and Slash',
  'description': 'This is a...',
  'type': 'ActiveSkill',
  'affects': 'target',
  'damage': 30,
  'damage-type': ['melee', 'iron'],
  'cooldown': 0,
  'mp-cost': 0
}

Or

{
  'name': 'Dual Wield'
  'description': 'Use two weapons!'
  'type': 'PassiveSkill'
  'tags': ['dual-wield']
}

Or

{
  'name': 'Health Potion',
  'description': 'Regain Health',
  'type': 'Item',
  'tags': ['destroy-on-use'],
  'target': 'self',
  'damage': -30,   # negative damage is health gain
  'inventory-space': 1
}

By making your code generic (rather than one function per skill/ability) and making all skills and items just data it will make your project easier to debug and items and skills easier to modify or add to.

share|improve this answer
    
Most definitely this. Make a few effect classes that resolve effects if you need them, like if you have a freezeChance attribute that gives a chance to apply frozen status, you can make a class/method that handles the freeze chance. Like that, you write each effect once and can then combine this to make new abilities! Best of all is that if you get non-programmer people interested in this they could easily create abilities without having to write code. – Pimgd Feb 25 at 15:35
    
So, instead of a class that handles abilities, I would have classes to handle abilities effects, and abilities themselves would be dictionaries with its effects? This could work well. Thanks for the advice. – inblank Feb 25 at 15:41
    
@inblank A useful note for this. Dictionaries can store a reference to a function even say, a class instance's function (eg player_fighter.hack_and_slash). – SuperBiasedMan Feb 25 at 16:33

Python 3 has enums, and you should try using them.

For example, you have attributes and equipment slots...

ATTRIBUTES = {'strenght': 0, 'constitution': 0, 'dexterity': 0, 'intelligence': 0, 'wisdom': 0, 'charisma': 0}

EQUIPMENT_SLOTS = {'head': None, 'chest': None, 'legs': None, 'boots': None}

but right now, there's a typo in your attributes. strenght is usually spelled strength in english. Were you to notice this typo one day and "fix" it, half your code would be pointing to strength and half your code would be pointing to strenght. By using enums, you can validate your attributes with syntax highlighting.

It's also easier to say that a helmet uses EQUIPMENT_SLOT.head rather than 'head' and hope you got it right (especially if you go in the direction of, say, morrowind, with left gauntlet and right pauldron and armor sets of 10+ pieces in general).


LEVEL_UP = 100  # initial XP to lvl up

By using all caps, it looks like you're declaring a constant.

Except it's not a constant.

def level_up(self):
    if self.exp >= self.LEVEL_UP:
        self.lvl += 1
        self.LEVEL_UP *= 1.25

You use it as both the constant AND the value, which then changes. The name LEVEL_UP loses a bit of its meaning as well in the statement self.LEVEL_UP *= 1.25 - it's no longer clear what it's for. Did you just raise the initial xp needed to level up, or the experience to the next level up?

Better would be setting xp_to_next_level in the constructor. That way your variable name expresses what it contains.


SKILLS = {'persuasion': 0, 'intimidation': 0, 'stealth': 0, 'perception': 0}

Uh... you seem to be overloading the word "skill". Is a skill one's skill/strength at persuasion, intimidation, stealth and so on, or is a skill the ability to perform a certain action (like "A powerful double attack", dubbed "hack and slash")?

Try to use one term per concept, that way when you're talking about skills in your comments I know you're talking about persuasion etc, and when you're talking about abilities you're talking about special moves.


Player class:

def __init__(self, armor_class):
    super().__init__(input("Tell us your name, hero:\n>"), 20, 10, {'gold': 10, 'torch': 1}, armor_class, 1)

If possible, try not to get input (view) whilst also creating objects that represent internal state (model). You see, if you wanted to test your player class, you couldn't, because making an instance requires getting a name. Other reasons include decoupling the view from the game (maybe you want to reuse your classes and skills and whatnot in a graphical sequel), being able to simulate battles... or loading a savefile!


    def equip_armor(self, armor_piece):
        """
        docstring
        """
        replace_equip = input(
            "Would you like to replace %s with %s?(Y/N)" % (self.EQUIPMENT_SLOTS['head'], armor_piece.name))
        if replace_equip.lower() == "y":
            self.EQUIPMENT_SLOTS['head'] = armor_piece
            return
        else:
            return

Here you've got a double return at the end of a function. Both cases end with the same line of code.

In those cases, you can just move the return out of the if-statement:

        if replace_equip.lower() == "y":
            self.EQUIPMENT_SLOTS['head'] = armor_piece
        return

Although in this case, you don't even need the return, as it is the end of the function:

    def equip_armor(self, armor_piece):
        """
        docstring
        """
        replace_equip = input(
            "Would you like to replace %s with %s?(Y/N)" % (self.EQUIPMENT_SLOTS['head'], armor_piece.name))
        if replace_equip.lower() == "y":
            self.EQUIPMENT_SLOTS['head'] = armor_piece

But maybe you want to include some text like "You equip the x" or "you keep wearing your y". It's useful to give feedback to players like that, I think.

share|improve this answer
    
Thanks, this code improvement tips are very useful! The overload of the 'skills' world came due the D&D translation, since skills for D&D are things like persuasion, stealth, etc. maybe, for the special moves I should use 'abilities' instead. – inblank Feb 25 at 14:54
    
By the way, would you think it's better to make attributes like strength, constitution, and so a dictionary or proper variables? – inblank Feb 25 at 14:59
    
@inblank Personally, I'm more of a Java programmer, and I'd prefer dictionary + getters in Java. If keeping everything public is more python's style, then you might get more syntax checking out of just raw variables. – Pimgd Feb 25 at 15:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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