0
\$\begingroup\$

We start off we the following Person class:

class Person:
    def __init__(self):
        self.moods = {
            "happy": 5,
            "angry": 5,
            "sad": 5
        }

    def updateStates(self, stateUpdates: {}):
        for mood, valChange in stateUpdates.items():
            self.moods[mood] += valChange

From here I implemented a restriction on what the values self.moods can be. I have implemented the restriction using descriptors. But I find it distasteful.

The following works. But I don't think it is worth it because now I have to append .__get__(instance=self, owner=None) to self.moods[<mood>] whenever I need to access a mood, or .__set__(instance=self, value=<newVal>) whenever I want to update one. This will quickly pollute my code and make it unreadable, which is why I am not a fan of this approach.

class OneDigitNumericValue():    # descriptor class
    def __set_name__(self, owner, name):
        self.name = name

    def __init__(self, defaultVal=5):
        self.value = defaultVal

    def __get__(self, instance, owner) -> object:
        return self.value

    def __set__(self, instance, value) -> None:
        if not (0 < value < 9) or int(value) != int:
            raise AttributeError("The value is invalid")
        self.value = value

class Person:
    moods = {
        "happy": OneDigitNumericValue(),
        "angry": OneDigitNumericValue(),
        "sad": OneDigitNumericValue()
    }

    def updateStates(self, stateUpdates: {}):
        for mood, valChange in stateUpdates.items():
            oldVal = self.moods[mood].__get__(self, owner=None)
            self.moods[mood].__set__(self, value=oldVal + valChange)
\$\endgroup\$
  • 1
    \$\begingroup\$ This code can't ever work. Have you actually ran your provided code? int(value) != int is always true, you always error on any input. \$\endgroup\$ – Peilonrayz Jan 25 at 22:53
  • \$\begingroup\$ Can you provide some examples on what input you expect this to work with. Because Person().updateStates({'happy': 1}) doesn't work. \$\endgroup\$ – Peilonrayz Jan 26 at 12:17
2
\$\begingroup\$

Your solution just doesn't work

  1. Your validation check always fails. Yes int(value) != int is always true. I don't think an instance of a class will ever be the class that it's an instance of.
  2. Your mutating a class variable. The tool you're utilizing forces you to take instances, normally there's a pretty good reason.

    If we remove the previous error and run your code we can easily see, there's no point in having more than one instance of a Player.

    >>> a = Person()
    >>> b = Person()
    >>> a.moods['happy'].__set__(a, 8)
    >>> b.moods['happy'].__get__(b, None)
    8  # Ok, that's not right...
    

I don't believe you've actually tried to code you've provided. And honestly what's the point in a single-data non-singleton class? The provided code is just untested garbage.

You're using it wrong

If it's not evident enough that you're fighting a tool that is pure sugar. Then you're not doing it right. The instance has to keep the state of the instance, and the descriptor has to interact with a varying amount of instances to interact with their state correctly.

If anything of the instance's leaks out into the descriptor, or vice versa. Then you're going to have a bad, bad time.

Since descriptors are to be tied onto classes I'm making a Mood class. Since the descriptor needs to have a place to store data. We can define that in the __init__ of Mood. To note, because you may not notice this by yourself. self._values can be mutated to bypass the validator. Using self._values[mood] = value is not how you update values, use setattr(self, mood, value).

In addition to fixing OneDigitNumericValue, you should make it take an argument that is the validator for the function. If you need another validator it'd be better and simpler to make a simple hard to mess-up function. Rather than making lots of potentially broken descriptors.

def single_digit(value):
    if 0 <= value <= 9:
        return
    raise ValueError("The value is invalid")


class Validator:
    def __init__(self, validator):
        self.validator = validator
        self.name = None

    def __set_name__(self, owner, name):
        self.name = name

    def __get__(self, instance, owner) -> object:
        return instance._values[self.name]

    def __set__(self, instance, value) -> None:
        self.validator(value)
        instance._values[self.name] = value


class Mood:
    happy = Validator(single_digit)
    angry = Validator(single_digit)
    sad = Validator(single_digit)

    def __init__(self):
        self._values = {}
        self.happy = 5
        self.angry = 5
        self.sad = 5

    def update_states(self, states):
        for mood, value in states.items():
            setattr(self, mood, value + getattr(self, mood))

And it actually works:

>>> a = Mood()
>>> b = Mood()
>>> a.happy = 3
>>> a.happy
3
>>> b.happy
5
>>> a.update_states({'happy': 3})
>>> a.happy
6
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

First of all, as per PEP8 I'd recommend you follow the naming conventions.

A couple of examples:

  • updateStates -> update_states
  • defaultValue -> default_value

I think you were headed in the right direction, but instead of overriding the __get__ and __set__ methods, I would just override the __add__ method, which would allow you to reuse the updateStates method from the first snippet. The __add__ method allows you to override the behavior of the + operator (https://docs.python.org/3/reference/datamodel.html#object.add).

Instead of int(value) != int to check whether value is an integer, you should use isinstance, which is the preferred way to check for the type of the variable in Python3.

Below I wrote a snippet incorporating the suggestions. As you can see, I pretty much reused your code, just added the __add__ method and the isinstance check.

class OneDigitNumericValue:
    def __init__(self, default_val=5):
        self.value = default_val

    def __add__(self, other_value):
        if not isinstance(other_value, int):
            raise AttributeError("The value is not an integer.")

        new_value = self.value + other_value

        if not (0 < new_value < 9):
            raise AttributeError("The value is not between 0 and 9.")

        return OneDigitNumericValue(new_value)


    def __repr__(self):
        return str(self.value)

class Person:
    moods = {
        "happy": OneDigitNumericValue(),
        "angry": OneDigitNumericValue(),
        "sad": OneDigitNumericValue()
    }

    def update_states(self, state_updates: {}):
        for mood, val_change in state_updates.items():
            self.moods[mood] += val_change

p = Person()
p.update_states({"happy": 1, "angry": 1})
print(p.moods)
p.update_states({"happy": 10, "angry": 1}) # error
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Great idea, but I don't think this a good approach. Now, whenever you add an integer to self.moods[mood], like +2, you are basically doing self.moods[mood]+=2. In other words, you get an unwanted (invisible) sideeffect that whenever you add an integer to self.moods[mood], self.moods[mood] is also (invisibly) reassigned to that value. Meaning, your sixth last line self.moods[mood] += val_change can be rewritten as self.moods[mood] + val_change. This unwanted sideeffect might have consequences down the road; especially if coworkers unknowingly adds a value to self.moods. \$\endgroup\$ – Sebastian Nielsen Jan 25 at 9:51
  • \$\begingroup\$ If that is the case, instead of mutating self.value and returning self from the __add__ method you can create a new object and return it: return OneDigitNumericValue(new_value). I edited the solution, to reflect this. \$\endgroup\$ – Rok Novosel Jan 25 at 9:54
  • \$\begingroup\$ Actually, instead of reassigning a value to self.value in __add__ , you can just return the new_value like showed here: pastebin.com/PYEsiCYu. If you run the code in the pastebin, you'll see it has the desired result without the unwanted sideeffect mentioned in my first comment. \$\endgroup\$ – Sebastian Nielsen Jan 25 at 10:06
  • \$\begingroup\$ Check my edited solution, I return a new object without modifying the original object. Also returning just the new_value variable, would mean that you would have integers as values in your moods dictionary, not OneDigitNumericValue objects. \$\endgroup\$ – Rok Novosel Jan 25 at 10:10
  • \$\begingroup\$ Mutating classvar's just won't end well with instances. Try making an second Person and you'll see that they have the exact same values all the time. \$\endgroup\$ – Peilonrayz Jan 25 at 23:57

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