zedr/clean-code-python

Wraping parameters into dictionary in "Function arguments (2 or fewer ideally)"

sitnarf opened this issue · 10 comments

sitnarf commented 
class Menu:
    def __init__(self, config: dict):
        title = config["title"]
        body = config["body"]
        # ...

menu = Menu(
    {
        "title": "My Menu",
        "body": "Something about my menu",
        "button_text": "OK",
        "cancellable": False
    }
)

From my experience, using a dictionary as a structure for holding data can lead to a big mess, when you have several complex structures like that. It is not typed checked and can't be annotated. I would use TypedDict instead.

Also, I think, it is not completely clear, how it solves the stated problem. The function still has the same amount of parameters. I think the more proper solution is to refactor the function/class into several smaller functions/classes and compose them (e.g. make_menu_content, make_button, etc.).

shabie commented

I also think that the comparison is honestly not equivalent. A function is getting X number of arguments where as the constructor gets a single dictionary. A function can work just as well with the dictionary 😄

takkaria commented

Yeah, this seems like a weirdly Java-y thing to recommend in Python! Especially when you have kwargs...

  • 👍1
mpkocher commented

I agree with @sitnarf comments. I believe the IO layer (i.e., a dict from JSON or similar) should be clearly decoupled from the core data model of the class. One possible mechanism to do this is using staticmethod or a class method.

For example.

class Menu:
    def __init__(self, title:str, body:str):
        self.title  = title
        self.body = body

    @staticmethod
    def from_dict(d:dict):
        return Menu(d['title'], d['body'])


def example():
    # perhaps loaded from JSON or defined as code
    # in your app
    raw_conf = dict(title="Title", body="Content")

    m1 = Menu.from_dict(raw_conf)
    # if you "trust" the source and don't want to
    # explicitly write a "from_dict" method
    # then you can pass the dict.
    m2 = Menu(**raw_conf)
    return m1, m2
  • 👍1
z00sts commented

Unpopular opinion: using "no-schema" dictionaries with key access/validation/assertion in constructor is similar with using no-sql databases: technically, you still have a contract (or scheme), but implicit. I usually use pure python dict subclasses with fixed key sets (or dataclasses in new python versions). Errors will be more detailed with TypeError on creating MyDictyType object than on construction call of an another object.

If your data has a schema, use a structure to keep it
You may be tempted to use a list (or tuple, if your language allows) to keep your data if it's simple -- like, say, only 2 fields.
But if you data has a schema -- it has a fixed format -- you should always use some structure to keep it, but it a struct or a class.

(https://blog.juliobiason.me/thoughts/things-i-learnt-the-hard-way/)

IaroslavR commented

I agree with @sitnarf comments. I believe the IO layer (i.e., a dict from JSON or similar) should be clearly decoupled from the core data model of the class. One possible mechanism to do this is using staticmethod or a class method.

For example.

class Menu:
    def __init__(self, title:str, body:str):
        self.title  = title
        self.body = body

    @staticmethod
    def from_dict(d:dict):
        return Menu(d['title'], d['body'])


def example():
    # perhaps loaded from JSON or defined as code
    # in your app
    raw_conf = dict(title="Title", body="Content")

    m1 = Menu.from_dict(raw_conf)
    # if you "trust" the source and don't want to
    # explicitly write a "from_dict" method
    # then you can pass the dict.
    m2 = Menu(**raw_conf)
    return m1, m2

May be better to use @classmethod here? Explanation

zedr commented

Thanks. These are all good suggestions. I've addressed them in d08459e. I'm closing this issue for now. Sorry it took so long.

sitnarf commented

@zedr no problem, thank you for addressing the issue! 😉

sitnarf commented

TypedDict unfortunately lacks default values. The problem can be mitigated by using @dataclass, but from my experience, using TypedDict is better, since it is much more clear and you can do syntax tricks like {**foo_dict, **bar_dict}.

One way how to mitigate the lack of default values would be to use factory functions:

def make_menu_config(
    title: str, body: str,
    button_text: str,
    cancellable: bool = False
) -> MenuConfig:
    return MenuConfig(*args, **kwargs)
IaroslavR commented

TypedDict unfortunately lacks default values. The problem can be mitigated by using @dataclass, but from my experience, using TypedDict is better, since it is much more clear and you can do syntax tricks like {**foo_dict, **bar_dict}.

One way how to mitigate the lack of default values would be to use factory functions:

def make_menu_config(
    title: str, body: str,
    button_text: str,
    cancellable: bool = False
) -> MenuConfig:
    return MenuConfig(*args, **kwargs)

IMO best alternative for everything :) - attrs lib. https://www.attrs.org/en/stable/why.html