zedr/clean-code-python

MenuConfig example mutating/setting class variables listed as Good

mpkocher opened this issue · 2 comments

I am surprised to see this example listed as "Good".

class MenuConfig:
    """A configuration for the Menu.

    Attributes:
        title: The title of the Menu.
        body: The body of the Menu.
        button_text: The text for the button label.
        cancellable: Can it be cancelled?
    """
    title: str
    body: str
    button_text: str
    cancellable: bool = False


def create_menu(config: MenuConfig):
    title = config.title
    body = config.body
    # ...


config = MenuConfig
config.title = "My delicious menu"
config.body = "A description of the various items on the menu"
config.button_text = "Order now!"
# The instance attribute overrides the default class attribute.
config.cancellable = True

create_menu(config)

I think there might be some misunderstand of how classes work in Python or there's perhaps a typo yielding a confusion on instance vs class.

Any key:type annotation on a class gets added to __annotations__. If the key:type has a default then it will be set as the default class variable. Any setter on the class will set any value (e.g., config.dragons = 34). I believe this practice should be discouraged.

Here's an example:

In [13]: class Record:
    ...:     alpha:int
    ...:     beta:int
    ...:     gamma:int = 1234
    ...:     
    ...:             

In [14]: r = Record

In [15]: r.alpha
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-15-b43492ebc57e> in <module>()
----> 1 r.alpha

AttributeError: type object 'Record' has no attribute 'alpha'

In [16]: r.__annotations__
Out[16]: {'alpha': int, 'beta': int, 'gamma': int}

In [17]: r.gamma
Out[17]: 1234

In [18]: r.dragons = 27

In [19]: r.more_dragons = 48

I would suggest that this Menu example should use an instance, not class, or the example should be marked as "Bad" in the text.

zedr commented

Thanks, @mpkocher. It is indeed a typo. It should be an instance, and not the class.

zedr commented

Fixed in 76e2f72. Added @mpkocher as a contributor in d4b7086. Thanks again.