trjhtr

I'm working on a cross-platform, implementation-independent Python library to make RPG-style game development easier, or at the very least more accessible.

This code is designed to run on Python 3.6 and above, and it has been tested with mypy and pytest. It currently lacks any custom types for the classes, as I don't know how to implement them with typing.NewType. I do not plan to support earlier versions of Python. I do not plan to attach this library to any specific game library, such as Pygame or PySDL2, although this may change down the line.

All data is currently stored in JSON files; I originally used XML, then TOML, but as the library kept growing I decided to just use JSON as it became clear that the way I implemented the items became very difficult to represent as either of those formats in a clean, concise way.

An example item would look like this:

"0": 
 "name": "Coins",
 "type": "item",
 "stackable": true,
 "examine": [
 "Lovely money!",
 "A stack of coins."
 ]
,
"1": 
 "name": "Wooden sword",
 "type": "weapon",
 "examine": "A dull wooden sword.",
 "atk": 5,
 "def": 3,
 "combine": [
 [2, 5]
 ]
,

where the item IDs are used as keys for each item. name represents the item's in-game name, type where it can be equipped (if at all), examine is a string giving information about the item (if non-stackable) or a list containing two strings, one with information and one with the current amount (if stackable), with the examine changing depending on the number of items on the stack.

There are also optional arguments, such as atk and def, which boost stats when equipped. combine is a list of possible ways to combine the item with other items to create a new item; the numbers used are all other item IDs, the final one which is the resulting item.

I just want feedback on my implementation, such as; is it readable enough? Are there enough comments? Was it a good idea to separate item data from code like this? Was using mixins a bad idea?

Here is my implementation:

class Item(ReprMixin, DataFileMixin):

 """ Class for generating item objects; used by Inventory and Player """

 EQUIPMENT = ('weapon',
 'head',
 'chest',
 'legs',
 'off-hand',)

 def __init__(self, id_num: int, **kwargs) -> None:

 """
 Initiates an Item object

 Arguments:
 - id_num: a unique integer representing the item to be created. Required.

 Optional keyword arguments:
 - file: name of, or path to, a file from which the item data is gathered.
 Defaults to the ITEM_FILE constant.
 - count: for a stackable item, sets how many items are on the stack
 - meta: metadata describing the item, defaults to None
 """

 # Get item data with DataFileMixin.get_item_by_ID()
 item_data = self.get_item_by_ID(
 id_num,
 file=kwargs.get('file', ITEM_FILE)
 )

 # Basic attributes every item has defined
 self.ID = int(id_num)
 self.name = item_data['name']
 self.slot = item_data['type']
 self.descriptions = item_data['examine']
 #NOTE: The item's actual description
 #is defined in the Item.description property!
 #This is due to the distinction between
 #normal and stackable items.

 # Attributes exclusive to wearable items
 if self.slot in self.EQUIPMENT:
 self.attack = item_data.get('atk', None)
 self.defence = item_data.get('def', None)
 self.specialAttack = item_data.get('specialAttack', None)

 # Miscellaneous optional attributes
 self.stackable = item_data.get('stackable', False)
 self.combinations = item_data.get('combine', None)

 self.metadata = kwargs.get('meta', None)
 if self.stackable:
 self._count = kwargs.get('count', 1)

 def __eq__(self, item):
 """ Compares the ID and metadata values of two items """
 return self.ID == item.ID and self.metadata == item.metadata

 def __lt__(self, item):
 try:
 return int(self.ID) < int(item.ID)
 except ValueError:
 if self.ID.isdigit() and not item.ID.isdigit():
 return True
 elif self.ID.isdigit():
 return self.ID < item.ID
 return False

 def __gt__(self, item):
 try:
 return int(self.ID) > int(item.ID)
 except ValueError:
 if self.ID.isdigit() and not item.ID.isdigit():
 return False
 elif self.ID.isdigit():
 return self.ID > item.ID
 return True

 @property
 def description(self):
 if not self.stackable:
 return self.descriptions
 examine = self.descriptions[0]
 if self._count >= ITEM_MAX_COUNT:
 examine = self.descriptions[1].format(self._count)
 return examine

Since they are referenced in the code, here are the implementations of DataFileMixin and ReprMixin:

from abc import ABCMeta

class DataFileMixin(metaclass=ABCMeta):
 """ Contains methods for getting game data from files """

 @staticmethod
 def _get_by_ID(ID: int, obj_type: str, file: str, file_format=DATA_FORMAT) -> dict:
 """ 'Low-level' access to filedata """
 with open(file) as f:
 if file_format == "json":
 data = json.load(f, parse_int=int, parse_float=float)
 elif file_format == "toml":
 data = toml.load(f)
 else:
 raise NotImplementedError(f"Missing support for opening files of type: file_format")
 return data[obj_type][str(ID)]

 def get_item_by_ID(self, ID: int, file=ITEM_FILE) -> dict:
 """ Returns a dictionary representation of a given item ID """
 return self._get_by_ID(ID, 'items', file)

 def get_enemy_by_ID(self, ID: int, file=ENEMY_FILE) -> dict:
 """ Returns a dictionary representation of a given enemy ID """
 return self._get_by_ID(ID, 'enemies', file)

 def get_npc_by_ID(self, ID: int, file=NPC_FILE) -> dict:
 """ Returns a dictionary representation of a given NPC ID """
 return self._get_by_ID(ID, 'NPCs', file)

 def get_entity_by_ID(self, ID: int, file=ENTITY_FILE) -> dict:
 """ Returns a dictionary representation of a given entity ID """
 return self._get_by_ID(ID, 'entities', file)


class ReprMixin(metaclass=ABCMeta):
 """ Automatically generates a __repr__-method for any class """

 def __repr__(self):
 """ Automatically generated __repr__-method """

 attributes = [f"key=value" if type(value) != str
 else f'key="value"'
 for key, value in vars(self).items()]
 v_string = ", ".join(attributes)
 class_name = self.__class__.__name__
 return f"class_name(v_string)"

And here are the global constants, defined in a separate configuration file:

from pathlib import Path

DATA_DIR = Path(__file__).parent / "data"
DATA_FORMAT = "json"
ITEM_MAX_COUNT = 10**5 #Used to change item description

ITEM_FILE = str(DATA_DIR / f"items.DATA_FORMAT")
ENTITY_FILE = str(DATA_DIR / f"entities.DATA_FORMAT")
ENEMY_FILE = str(DATA_DIR / f"enemies.DATA_FORMAT")
NPC_FILE = str(DATA_DIR / f"npcs.DATA_FORMAT")
QUEST_FILE = str(DATA_DIR / f"quests.DATA_FORMAT")
OBJECT_FILE = str(DATA_DIR / f"objects.DATA_FORMAT")

Was using mixins a [good] idea?

Yes, it helps with Single Responsibility. It lets a mixin class do One Thing well.

Separating data from code is good thing, and JSON is quite convenient. But it doesn't support comments. Consider integrating with https://pypi.python.org/pypi/jsoncomment/0.2.2

Item 0 (coins) seems to be missing a quantity attribute. In its format string, might be quantity. On item 1, +5 attack and +3 defend for the sword were clear enough, but I was hoping for a comment on "combine". Your accompanying prose description was helpful. Using capitals or mixed case in name for non-proper-noun items seemed odd.

EQUIPMENT = ('weapon',
 'head',
 'chest',
 'legs',
 'off-hand',)

The trailing comma after 'off-hand' is good, but don't follow it with close paren, put paren on its own line. The point of trailing comma is to minimize source repo diffs.

It appears that EQUIPMENT wants to be an Enum.

 """
 Initiates an Item object

No, that is not a useful comment. We already know from the signature that we're in a ctor. In one line you should be telling us what secret an Item guards, what Single Responsibility it takes on so other code doesn't have to.

 Arguments:
 - id_num: a unique integer representing the item to be created. Required.

This comment is fine as it is, but consider deleting it since it says nothing beyond what the signature already told us. The identifier id_num is a thoughtful one, trying to spell out for the Gentle Reader that it is numeric. But this is usual, and the signature helpfully spelled out that it is of int. Consider renaming to simply id.

Ok, I suggested quantity above, I see you settled on the shorter count, that's cool.

 Optional keyword arguments:
 - file: name of, or path to, a file from which the item data is gathered.
 Defaults to the ITEM_FILE constant.
 - count: for a stackable item, sets how many items are on the stack
 - meta: metadata describing the item, defaults to None

The file comment is tortuous and much too long. Say it's filespec of item data and be done with it. Or be specific, and nail it down to JSON or whatever. The defaulting remark is bizarre. Why, oh why didn't we declare file in the signature, and give the default value there as well?

 file=kwargs.get('file', ITEM_FILE)

This expression is redundant with simply file if the signature uses the standard idiom of def __init__(self, id: int, file=ITEM_FILE, **kwargs).

I found the stack comment a little surprising, as I had in mind that "stackable" was used to compose e.g. a Coins item and a Bills (or Jewels) item. The comment suggests that stackable instead relates to descriptions and player verbs valid with certain items.

The meta remark is not helpful, as "metadata describing the item" gets me no closer to understanding what would be valid and what it would mean. Consider using the more verbose metadata. Definitely consider putting metadata=None in the signature.

 item_data = self.get_item_by_ID(

The "_data" part is vague and unhelpful. Please just call it an item.

 self.ID = int(id_num)

That's not quite a manifest constant, so pep8 asks you to please spell it self.id.

 self.slot = item_data['type']

slot is a perfectly nice identifier, but I don't understand why we're gratuitously renaming. If type wasn't right, it should have been slot from the get go. It appears that self.type_ would be the more natural identifier here (since python already defines type).

 #NOTE: The item's actual description
 #is defined in the Item.description property!
 #This is due to the distinction between
 #normal and stackable items.

You lost me there. The semantics on 'description' seemed simple enough, but now we have muddied the waters.

 # Attributes exclusive to wearable items

This suggests that the current class might be called BaseItem, and that WearableItem might inherit from that, and hold the attack / defense code. And a StackableItem would default to count=1.

 self.specialAttack = item_data.get('specialAttack', None)

Define the JSON attribute spellings any way you like, but pep8 asks that you assign to self.special_attack. Please run flake8 and follow its advice.

The item_data.get('stackable', False) and item_data.get('combine', None) expressions suggest that stackable=False, combine=None belong in the signature.

def __eq__(self, item):
 """ Compares the ID and metadata values of two items """
 return self.ID == item.ID and self.metadata == item.metadata

I don't understand that at all. We can have an item 7 with empty metadata, and also have an item 7 with foo=bar? That's crazy. An id should uniquely identify the thing. Maybe what we have here is more like a SKU or a part_number?

I don't understand lt and gt, either, given that a well-formed Item will always have a numeric id. Maybe that wasn't always true, and we're seeing some remnants of an older codebase that should be edited? Consider adding this to the Item ctor: assert 0 + id == id, so attempts to use a string id will throw.

 if self._count >= ITEM_MAX_COUNT:

A max count suggests a limit on what a well-formed Item may hold. Consider defining a one-line predicate helper, perhaps is_a_lot() or is_large_amount(), to control which of the alternate descriptions is used.

""" Contains methods for getting game data from files """

Yay! This is a helpful comment.

 if file_format == "json":

Consider deleting that parameter, and simply asking if file.endswith('.json').

Use lru_cache to memoize each parsed JSON file.

The various getters appear redundant. The caller might simply pass in a string or Enum. You want a dict that maps from Enum to filename, or an Enum that can itself return the proper filename. The docstrings might be elided as they tell us little beyond what the signature says.

I'm not exactly sure what you implemented the __gt__ and __lt__ methods for. Is this just to be able to sort a list of items (by some arbitrary number, the ID)?

I would think what might be more helpful (either in addition to this, or on its own), is to be able to compare stack sizes of equal items. This way player1.gold > player2.gold, would make sense (where both players have the gold item, but with different stack sizes).

You should also think about if equality should test for this as well. In other words, is one gold coin and 1000 gold coins the same or not? Whatever your answer to this question is, you should probably offer both comparison methods to the user of the class (and choose one for __eq__).

Another note on your DataFileMixin. You are currently reading the whole data file in, whenever you access an element. It will be faster (especially when the file grows, which it will, if you actually implement a game with this) to cache the file content and serve the element from that. Of course this requires the data to fit into memory, but so does your current implementation.

You might want to add a check to see if the file has been modified since the last read (in which case you have to read it again, of course), maybe using the modification time. This may not be fool-proof, for example if the user changes the system time or changes between time zones or something similar, but should be good enough.

If the file does not fit into memory anymore, you have to read the file sequentially until you find the element you are looking for, which would be going back again to reading the file on every access (but you should still cache elements you have looked up once).