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

Conversation

Nostrademous
Copy link
Contributor

No description provided.

Copy link
Member

@ppoelzl ppoelzl left a comment

Choose a reason for hiding this comment

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

There are multiple nits where I've only commented on the first appearing one. If you have any questions regarding the proposed refactorings, please let me know!

@@ -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?

@@ -0,0 +1,47 @@
#

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.


def addDependency(dep1: str, dep2: str):
global _dependencies, _dependent_vars
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.

_dependent_vars = set()

def addDependency(dep1: str, dep2: str):
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.

attr = f"{mod.type.lower()}_{mod.name.lower()}"
try:
self.__delattr__(f"{attr}")
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.


@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"{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

if not entry in self.db[name][type]:
self.db[name][type].append(entry)

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"])
```

self.conditionals = conditionals
self.tags = tags
self.process()

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants