-2
\$\begingroup\$

I've an object that looks like this, and users have been populating the attributes post-initialization, e.g.

class ObjectThingy:
    def __init__(self):
        self.att1 = set() # set(tuple)
        self.att2 = {} # dict(str:str)

attribute1 = [('foo', 'bar'), ('soap', 'bar'), ('gastro', 'bar')]
attribute2 = {'hello': 'world', 'foo': 'bar'}

ot = ObjectThingy()
ot.att1 = attribute1
ot.att2 = attribute2

And now I'm trying to write an object loader without changing the number of variables in __init__ (maybe optional *args and **kwargs are okay though). I came up with a staticmethod that returns the an instantiation of the class:

import json

attribute1 = [('foo', 'bar'), ('soap', 'bar'), ('gastro', 'bar')]
attribute2 = {'hello': 'world', 'foo': 'bar'}

with open('object.json', 'w') as fout:
    _json = {'att1': attribute1, 'att2': attribute2}
    json.dump(_json, fout)


class ObjectThingy:
    def __init__(self):
        self.att1 = set() # set(tuple)
        self.att2 = {} # dict(str:str)
        
    @staticmethod
    def load(jsonfile):
        with open(jsonfile) as fin:
            _json = json.load(fin)
            
        att1 = set(map(tuple, _json['att1']))
        att2 = _json['att2']
        
        ot = ObjectThingy()
        ot.att1.update(att1)
        
        for k,v in att2.items():
            ot.att2[k] = v
        
        return ot

It kind of works but seems sort of off when I'm trying to just populate the object attributes when loading instead of initializing them. And I try another pattern where I minimize my loading function and offload the operations to checks using kwargs during initialization:

class ObjectThingy:
    def __init__(self, **kwargs):
        
        if 'att1' in kwargs and all(type(t) == tuple for t in kwargs['att1']):
            self.att1 = set(map(tuple, kwargs['att1']))
        else:
            self.att1 = set() # set(tuple)
            
        if 'att2' in kwargs and isinstance(kwargs['att2'], dict):
            self.att2 = kwargs['att2']
        else:
            self.att2 = set() # set(tuple)
            
        
    @staticmethod
    def load(jsonfile):
        with open(jsonfile) as fin:
            _json = json.load(fin)
            
        att1 = set(map(tuple, _json['att1']))
        att2 = _json['att2']
        
        return ObjectThingy(att1=att1, att2=att2)

ot3 = ObjectThingy.load('object.json')

print(ot3.att1)
print(ot3.att2)

Are there specific use-cases for which the previous/latter solution is preferred?

Are there other idiomatic patterns that we can use for similar object loading functions (pickling is not possible in my task)?

Is there any other possible way to load the object with/without staticmethod?

\$\endgroup\$
4
  • 5
    \$\begingroup\$ You have 6 answers using classmethod on SO, one dating back to '13. Is your question genuine? \$\endgroup\$ Commented Jul 7, 2024 at 12:16
  • \$\begingroup\$ Yes it's genuine... \$\endgroup\$ Commented Jul 8, 2024 at 10:05
  • \$\begingroup\$ att1 and att2 should be constructor params. Your own factory function violates the class integrity when you do ot.att1.update(att1) and ot.att2[k] = v. -> def __init__(self, att1=None, att2=None): self.att1 = att1 or set(); self.att2 = att2 or dict(). By providing defaults you don't run into conflicts with existing code. \$\endgroup\$ Commented Jul 9, 2024 at 12:54
  • \$\begingroup\$ And this is bad code: ot = ObjectThingy(); ot.att1 = attribute1; ot.att2 = attribute2. So probably it would be an good idea to provide accessors for att1 and att2. \$\endgroup\$ Commented Jul 9, 2024 at 12:56

1 Answer 1

Reset to default
4
\$\begingroup\$

comment vs. annotation

        self.att1 = set() # set(tuple)
        self.att2 = {} # dict(str:str)

Please don't write comments just for humans. Tell mypy about your intention, as well!

        self.att1: set[tuple] = set()
        self.att2: dict[str, str] = {}

Also: a tuple of what?

And ObjectThingy is a terrible name for code being submitted for review.

type stability

        self.att1 = set() # set(tuple)
...
ot.att1 = [('foo', 'bar'), ('soap', 'bar'), ... ]

Please don't tell me you're defining a set, and then jam a list in there.

If we had a spec, and a real name rather than att1, we could tell what Author's Intent was. As written, it is opaque.

(I know, I know, by default we can't json.dump() a set. But that's the serializer's responsibility to worry about.)

discarding existing container

ot.att1 = attribute1
ot.att2 = attribute2

You're discarding the previous container and replacing it with a new container. It would be more natural to pass a pair of containers in to the __init__ ctor. Or to add elements to existing containers:

list(map(ot.att1.add, attribute1))
ot.att2.update(attribute2)

This is certainly one way to update an existing container:

        for k,v in att2.items():
            ot.att2[k] = v

But prefer to .update() the dict.

type stability redux

What is going on here?!?

        ot.att1.update(att1)

You told me it's a set, then a list, and now you seem to suggest it's a dict?!? Make up your mind. Write a Specification for your Use Case. Proposing hypothetical code does not appear to be helping you present a cogent argument about code details.

seems sort of off when I'm trying to just populate the object attributes when loading

This concern make sense. It seems to suggest that you'd benefit from a model or schema datastructure, which your loader knows how to parse.

fatal error

        if 'att1' in kwargs and ... :
            self.att1 = set(map( ...
        else:
            self.att1 = set()

That else clause seems undesirable. That is, we have a precondition that caller shall offer a well-formed att1. If that is violated, it would be better to raise fatal error. In that way if we write a bug, it is a "shallow" bug, easily discovered and fixed. As written, it looks like this code will mask buggy callers, so no one notices the trouble and no one fixes it.

Similarly for att2. What I'm driving at is the isinstance() conjunct seems like too much LBYL. If the caller's input quacks like a duck then good, else we encounter some fatal error like "has no attribute 'update'". If caller supplied an attN, it must be a well-formed attribute and not an arbitrary object of some random type.

other idiomatic patterns

Short circuiting or tends to be pretty handy for defaulting:

        self.att2 = att2 or {}

This works nicely with att2=None keyword default, and with optional attributes that may be missing from a .json input file.

json loading ctor

Rather than making .load() a staticmethod, you might prefer to make it an ordinary method which adds elements to your attribute containers. So calling it several times on "in1.json" and "in2.json" would keep growing the existing containers.

any other way to load the object without staticmethod?

Since "init from .json" seems important to your Use Case, consider adopting one of these signatures:

    def __init__(self, jsonfile):
        """Initialize a pair of attributes that were serialized to disk."""

    def __init__(self, jsonfile=None, att1=None, att2=None):
        """Caller should specify exactly one of
            1. jsonfile
            2. both attributes
        """

Caller might even find it convenient to pass in an io.StringIO instead of a Path. That would admit of making small tweaks or additions to a serialized form that caller retrieved from disk. Since the OP only contains hypothetical example code, it is impossible to tell what appropriate tradeoffs for your real codebase would be.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.