Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add some basic classes #13

Draft
wants to merge 10 commits into
base: dev
Choose a base branch
from
47 changes: 47 additions & 0 deletions src/classes/Dependency.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does every file begin with "#" here?


from itertools import chain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer importing full modules. E.g.:

import itertools

This helps in understanding where imported obhects come from.
For the typing module, prefer to only import individual objects though. E.g.:
Correct:

from typing import Union

Wrong:

import typing

This helps in keeping type signatures readable.


_dependencies = {}
_dependent_vars = set()

def addDependency(dep1: str, dep2: str):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functions, variables and also modules should be named in snake_case. There are a slot of style violations in this PR. I recommend reading https://www.python.org/dev/peps/pep-0008, the Python style guide.

global _dependencies, _dependent_vars
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid global variables where possible. Generally, you can always rewrite your code to share objects by other means instead. That said, in this case this seems fine for a prototype. Note that you only need to declare variables as global of you want to change them globally from a local scope. You can always read variables from outer scopes, so it is unnecessary to declare _dependencies as global here.

if dep1 in _dependencies.keys():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When iterating over a dict, you do not need to call the key method. dict iteration is already over its keys by default.

if not dep2 in _dependencies[dep1]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Membership tests should use not in. E.g.:

if dep2 not in _dependencies[dep1]:

_dependencies[dep1].append(dep2)
#print(f"Appending DEP: {dep1} -> {dep2}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid commiting commented out code. It does not age well. In fact, it doesn't age at all. 😉

else:
_dependencies[dep1] = [dep2]
#print(f"Adding DEP: {dep1} -> {dep2}")
_dependent_vars = set(chain.from_iterable(_dependencies.values()))
#print("DV: \n", _dependent_vars)
Comment on lines +8 to +18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting from this code,

def add_dependency(dep1: str, dep2: str) -> None:
    global _dependent_vars
    if dep1 in _dependencies:
        if dep2 not in _dependencies[dep1]:
            _dependencies[dep1].append(dep2)
    else:
        _dependencies[dep1] = [dep2]
    _dependent_vars = set(itertools.chain.from_iterable(_dependencies.values()))

we could refactor it like so:

def add_dependency(dep1: str, dep2: str) -> None:
    global _dependent_vars
    if dep1 not in _dependencies:
        _dependencies[dep1] = []
    if dep2 not in _dependencies[dep1]:
        _dependencies[dep1].append(dep2)
    _dependent_vars = set(itertools.chain.from_iterable(_dependencies.values()))

Which makes it more obvious that we want to use a defaultdict here:

import collections

_dependencies = collections.defaultdict(list)

def add_dependency(dep1: str, dep2: str) -> None:
    global _dependent_vars
    if dep2 not in _dependencies[dep1]:
        _dependencies[dep1].append(dep2)
    _dependent_vars = set(itertools.chain.from_iterable(_dependencies.values()))

Now, since our dependencies don't need to be ordered, we can change the default list to a set:

import collections

_dependencies = collections.defaultdict(set)

def add_dependency(dep1: str, dep2: str) -> None:
    global _dependent_vars
    _dependencies[dep1].add(dep2)
    _dependent_vars = set(itertools.chain.from_iterable(_dependencies.values()))


def getDependentVars():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid methods and functions starting with get_ or set_. E.g.: dependencies() is preferabe over get_dependencies().

return _dependent_vars

def getDependencies():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having these "getter" functions, it's preferable to have public module members and refactor the code if you would need to change them significantly. Refactoring code is easy in Python, so "future proofing" of this kind is not needed and detrimental to code clarity.

return _dependencies

class Dependency:
def _reset_dependent_vars(self, name: str):
#print(f"DEP\n{_dependencies}")
for var in _dependencies[name]:
#print(f"Resetting: {var} (due to {name})")
try:
super().__delattr__(f"{var}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var is already a str, so there's no need to f-string format it.

except:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bare except clauses are an antipattern. They mask real errors that affect other parts of the program, since all exceptions are captured, even ones we're not interested in. __delattr__ raises AttributeError if it can't delete an attribute, so the appropiate exception handling code is

except AttributeError:

#print(f"FAIL")
pass
if var in _dependencies:
self._reset_dependent_vars(var)

def __setattr__(self, name: str, value):
global _dependencies, _dependent_vars
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, these globals are not needed.

if name in _dependent_vars:
raise AttributeError("Cannot set this value.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error would be more helpful if it specified the attribute that cannot be set.

if name in _dependencies:
self._reset_dependent_vars(name)
name = f"_{name}"
#print(f"Setting {name} to {value}")
super().__setattr__(name, value)
68 changes: 68 additions & 0 deletions src/classes/ModDB.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#

from Modifiers import Modifier

class ModifierDatabase:
def __init__(self, owner = None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

owner is missing its type hint.

self.owner = owner
self.clear()

def addEntry(self, entry: Modifier):
name = entry.name
type = entry.type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is a Python builtin. Do not shadow builtins, as they cease to be available in the current scope after that.
If you want to use a name that is already reserved for a builtin, suffix it with an underscore. E.g.: type_, id_, etc.
For private variables, this would become _type, _id instead.

if name not in self.db.keys():
self.db[name] = dict()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer literals where possible. In this case:

self.db[name] = {}

if type not in self.db[name].keys():
self.db[name][type] = [entry]
else:
if not entry in self.db[name][type]:
self.db[name][type].append(entry)
Comment on lines +10 to +19
Copy link
Member

@ppoelzl ppoelzl Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting from this cleaned up example, we can again leverage defaultdict to get rid of much of the complexity in this method:

def __init__(self, owner: Any = None) -> None:
    self.owner = owner
    self.db = {}

def add_entry(self, entry: Modifier) -> None:
    name = entry.name
    type_ = entry.type_
    if name not in self.db:
        self.db[name] = {}
    if type not in self.db[name]:
        self.db[name][type_] = [entry]
    else:
        if entry not in self.db[name][type_]:
            self.db[name][type_].append(entry)

Now if we refactor the resulting code a bit, it again becomes more obvious that we can use another, this time nested, defaultdict:

import collections

def __init__(self, owner: Any = None) -> None:
    self.owner = owner
    self.db = collections.defaultdict(dict)

def add_entry(self, entry: Modifier) -> None:
    name = entry.name
    type_ = entry.type_
    if type not in self.db[name]:
        self.db[name][type_] = [entry]
    else:
        if entry not in self.db[name][type_]:
            self.db[name][type_].append(entry)

So this time we want to have a defaultdict(list) in a defaultdict itself. But defaultdict expects a callable as argument, and it itself is not callable.

import collections

def __init__(self, owner: Any = None) -> None:
    self.owner = owner
    self.db = collections.defaultdict(dict)

def add_entry(self, entry: Modifier) -> None:
    name = entry.name
    type_ = entry.type_
    if type not in self.db[name]:
        self.db[name][type_] = []
    if entry not in self.db[name][type_]:
        self.db[name][type_].append(entry)

lambda to the rescue!

import collections

def __init__(self, owner: Any = None) -> None:
    self.owner = owner
    self.db = collections.defaultdict(lambda: collections.defaultdict(list))

def add_entry(self, entry: Modifier) -> None:
    name = entry.name
    type_ = entry.type_
    if entry not in self.db[name][type_]:
        self.db[name][type_].append(entry)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases like this, when i'm adding elements to a collection, i like to use variadic arguments in a function. That tends to save me a lot of lines later on.


def getBase(self, name: str):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To come back to this after reviewing the player module:
The calculcations for individual mods should happen here, not in the Player class.

Implementation:

import math
from typing import Union

def base(self, name: str) -> int:
    return sum(entry.value for entry in self.db[name]["BASE"])

def flat(self, name: str) -> int:
    return sum(entry.value for entry in self.db[name]["FLAT"])

def inc(self, name: str) -> Union[float, int]:
    return sum((entry.value for entry in self.db[name]["INC"]), 1) / 100

def more(self, name: str) -> Union[float, int]:
    return math.prod(1 + entry.value / 100 for entry in self.db[name]["MORE"])
```

retVal = 0
if name in self.db.keys() and "BASE" in self.db[name].keys():
for entry in self.db[name]["BASE"]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stringly typed variables are a problem for gradually typed code bases. Consider refactoring "BASE", "FLAT", "INC" and "MORE" into an enum.

retVal += entry.getValue(self.owner)
return retVal
return 0
Comment on lines +21 to +27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have defaultdicts now, this method becomes a one-liner:

  def base(self, name: str) -> int:
      return sum(entry.value for entry in self.db[name]["BASE"])


def getFlat(self, name: str):
retVal = 0
if name in self.db.keys() and "FLAT" in self.db[name].keys():
for entry in self.db[name]["FLAT"]:
retVal += entry.getValue(self.owner)
return retVal
return 0

def getInc(self, name: str):
retVal = 0
if name in self.db.keys() and "INC" in self.db[name].keys():
for entry in self.db[name]["INC"]:
retVal += entry.getValue(self.owner)
return retVal
return retVal

def getMore(self, name: str):
retVal = 0
if name in self.db.keys() and "MORE" in self.db[name].keys():
for entry in self.db[name]["MORE"]:
retVal += entry.getValue(self.owner)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For "more" multipliers (hence the name) you need to calculate the product, not the sum.

return retVal
return retVal

def clear(self):
self.db = dict()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always initialise instance attributes in __init__ and nowhere else. In this case, clear is extra misleading, since it doesn't quite clear the mod DB. If you reassign self.db, all old references to it will still exist and the old mod DB might not get garbage collected at all. clear should call the clear method of the underlying collection instead.

def __init__(self, owner: Any = None) -> None:
    self.owner = owner
    self.db = {}

def clear(self) -> None:
    self.db.clear()


def test():
db = ModifierDatabase()
mod1 = Modifier("Health", "BASE", 12, "", tags = { "type": "Multiplier", "var": "Level" })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no whitespace around kwargs bindings.

mod2 = Modifier("Health", "BASE", 13, "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an usability perspective, it would be nice to provide the source as a default parameter, making it optional.

db.addEntry(mod1)
db.addEntry(mod2)
import pprint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import statements should be at the top of the respective scope, so here this should go directly after the function signature.

pprint.pprint(db.db)

print("BASE: " + str(db.getBase("Health")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use f-strings for formatting here. This also allows to get rid of the explicit string conversion.


if __name__ == "__main__":
test()
44 changes: 44 additions & 0 deletions src/classes/Modifiers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#

from Dependency import addDependency, _dependencies
from math import floor

class Modifier:
def __init__(self, name: str, type: str, value, source: str, conditionals: set = set(), tags: dict = {}):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function signature doesn't do what you think it does. In Python, function signatures are run once at the time of creation. This means that after that, if you have mutable default arguments, all invocations of the functions share their mutable arguments, leading to hard to find bugs.

Canonically, you would solve this by providing a sentinel default value, e.g.:

from typing import Optional

def __init__(self, conditionals: Optional[set[...]] = None, tags: Optional[dict[..., ...]] = None) -> None:
    if conditionals is None:
        conditionals = set()
    if tags is None:
        tags = {}

self.name = name
self.type = type
self.value = value
self.source = source
self.conditionals = conditionals
self.tags = tags
self.process()
Comment on lines +6 to +14
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all this initialiser does is set some attributes, this is a perfect use case for dataclasses:

import dataclasses
from typing import Union

@dataclasses.dataclass
class Modifier:
    name: str
    type_: str
    value: Union[float, int]
    source: str = ""
    conditionals: set = dataclasses.field(default_factory=set)
    tags: dict = dataclasses.field(default_factory=dict)
    
    def __post_init__(self) -> None:
        self.process()

Note the default_factory to avoid the problems with mutable default arguments described above.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since modifiers in our mod DB are always unique by ID and do not need to be ordered, we could simplify the code in mod_db.ModifierDatabase.add_entry by switching from a list for leaf nodes to a set. This nets us O(1) insertion over O(n) insertion accounting for the member check, as well as allowing us to get rid of a conditional in our code. The only complication is that we need to make modifiers.Modifier hashable.

Since we have decided that modifiers should always be inequal to every other modifier than themselves, this is possible to implement correctly even though modifiers.Modifier has mutable attributes:

class Modifier:
    ...

    def __eq__(self, other: Modifier) -> bool:
        return self is other

    def __hash__(self) -> int:
        return id(self)

This allows us to turn mod_db.ModifierDatabase into:

class ModifierDatabase:
    def __init__(self) -> None:
        self.db = collections.defaultdict(lambda: collections.defaultdict(set))

    def add_entry(self, entry: modifiers.Modifier) -> None:
        name = entry.name
        type_ = entry.type_
        self.db[name][type_].add(entry)

and subsequently:

def add_entry(self, entry: modifiers.Modifier) -> None:
    self.db[entry.name][entry.type_].add(entry)

def process(self):
if 'type' in self.tags.keys() and self.tags['type'] == "Multiplier":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Python, prefer EAFP (easier to ask for forgiveness than permission) over LBYL (look before you leap). So instead of checking whether a key exists, just try to access it and handle the exception if it doesnt exist.

addDependency(self.tags['var'].lower(), f"{self.type.lower()}_{self.name.lower()}")
addDependency(f"{self.type.lower()}_{self.name.lower()}", f"max_{self.name.lower()}")
#print(self)

def getValue(self, caller = None):
if caller:
#print(f"getting var: {self.name}:{self.type}")
if 'type' in self.tags.keys() and self.tags['type'] == "Multiplier":
var = f"{self.tags['var'].lower()}"
l = caller.__getattribute__(var)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid variable names like lowercase "L" that could be mistaken for other characters, depending on the font used.

#print(f"{var}: {l}")
return floor(self.value * l)
return self.value
return self.value

def __repr__(self):
ret = f"{self.name}:{self.type}\n"
ret += f"Value: {self.value}\n"
ret += f"{self.tags} -- {self.source}"
return ret
Comment on lines +34 to +37
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can take advantage of implicit string concatenation here.


def test():
m = Modifier("Health", "BASE", 12, "", tags = { "type": "Multiplier", "var": "Level" })
print(m)

if __name__ == "__main__":
test()
114 changes: 114 additions & 0 deletions src/classes/Player.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
#

from functools import cached_property
from math import floor

from Dependency import Dependency, getDependencies
from ModDB import ModifierDatabase
from Modifiers import Modifier

class Player(Dependency):
def __init__(self, level, strength):
self.modDB = ModifierDatabase(self)
self._level = level

self.addMod(Modifier("Health", "BASE", 12, "Base Per Level", tags = { "type": "Multiplier", "var": "Level" }))
self.addMod(Modifier("Health", "BASE", 0.5, "Base Per Strength", tags = { "type": "Multiplier", "var": "Max_Strength"}))
self.addMod(Modifier("Strength", "BASE", strength, "Starting"))

def addMod(self, mod: Modifier):
self.modDB.addEntry(mod)
attr = f"{mod.type.lower()}_{mod.name.lower()}"
try:
self.__delattr__(f"{attr}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, attr is already a str.

except:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, avoid bare except clauses.

#print("FAIL")
pass
if attr in getDependencies():
self._reset_dependent_vars(attr)

@cached_property
def base_health(self):
#ret = 38 + self.level * 12 + floor(self.max_strength / 2) + self.flat_health
ret = 38 + self.modDB.getBase("Health") + self.flat_health
print(f"BASE Health calculated: {ret}")
return ret

@cached_property
def max_health(self):
ret = floor(self.base_health * (1 + self.inc_health / 100) * (1 + self.more_health / 100))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summing up all like modifiers looks more like a responsibility of the mod DB than the Player class. Also the calculations for "More" multipliers are wrong if more than one "More" multiplier is present. You need to calculate 1 + mod / 100 for each modifier, prior to calculation the product of them.

print(f"Total Health calculated: {ret}")
return ret

@cached_property
def base_strength(self):
ret = self.modDB.getBase("Strength")
print(f"BASE Strength calculated: {ret}")
return ret

@cached_property
def max_strength(self):
ret = floor((self.base_strength + self.flat_strength) * (1 + self.inc_strength / 100) * (1 + self.more_strength / 100))
print(f"Total Strength calculated: {ret}")
return ret

@property
def level(self):
return self._level

@cached_property
def flat_health(self):
ret = self.modDB.getFlat("Health")
print(f"FLAT Health calculated: {ret}")
return ret

@cached_property
def more_health(self):
ret = self.modDB.getMore("Health")
print(f"MORE Health calculated: {ret}")
return ret

@cached_property
def inc_health(self):
ret = self.modDB.getInc("Health")
print(f"INC Health calculated: {ret}")
return ret

@cached_property
def flat_strength(self):
ret = self.modDB.getFlat("Strength")
print(f"FLAT Strength calculated: {ret}")
return ret

@cached_property
def more_strength(self):
ret = self.modDB.getMore("Strength")
print(f"MORE Strength calculated: {ret}")
return ret

@cached_property
def inc_strength(self):
ret = self.modDB.getInc("Strength")
print(f"INC Strength calculated: {ret}")
return ret

def test():
player = Player(1, 20)
#print(f"{player.base_health}\n")
print(f"{player.max_health}\n")

player.level = 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will shadow the level property, since it cannot be set without providing a setter method. E.g.:

@level.setter
def level(self, value: int) -> None:
    self._level = value

print(f"{player.max_health}\n")

player.addMod(Modifier("Health", "MORE", 100, ""))
print(f"{player.max_health}\n")

player.addMod(Modifier("Strength", "FLAT", 100, ""))
player.addMod(Modifier("Strength", "INC", 30, ""))
player.addMod(Modifier("Strength", "MORE", 15, ""))
player.addMod(Modifier("Health", "FLAT", 500, ""))
#print(f"{player.max_strength}\n")
print(f"{player.max_health}\n")

if __name__ == "__main__":
test()
Empty file added src/classes/__init__.py
Empty file.