File class and large constructor

Question

I have some dispute with a colleague. I work with some REST-service, from which I get always same type data.

class File():
 def __init__(self, name, created, modified, media_type, path, md5, type, mime_type, size,
 public_key=None, public_url=None, preview=None):
 self.public_url = public_url
 self.preview = preview
 self.name = name
 self.created = created
 self.modified = modified
 self.path = path
 self.type = type
 self.media_type = media_type
 self.md5 = md5
 self.mime_type = mime_type
 self.size = size

The following code snippet demonstrate, how I work with File class:

def get_list_of_all_files(self):
 url = self._base_url + "/resources/files"
 r = requests.get(url, headers=self.base_headers)
 json_dict = r.json()
 files = []
 for item in json_dict["items"]:
 f = File(**item)
 files.append(f)
 return files

My colleague says that the constructor is very large, and this code is horrible. Is he right? How should look like this code?

Answer 1

There are three ways I know of to deal with large container classes:

1. large constructors
2. post-constructor modifiers
3. sub-categories

None of them are 'nice'.

Large Constructor

Your code is an example of the large constructor.

Post Construction

The post-constructor modifier system will have a bunch of 'set' methods, and you call them like:

 myFile.setSize(...);
 myFile.setMD5(...);

Sub-Categories

The sub-cateogory option is not awful, it is done by grouping related parameters together. For example, the two times could be one class:

class FileTimes():
 def __init__(self, created, modified):
 self.created = created
 self.modified = modified

then the file locations could be grouped as well

class FileLocation():
 def __init__(self, path, public_url=None):
 self.public_url = public_url
 self.path = path

FileGroup would be, perhaps, the various 'types', like type, media_type, mime_type.

Once you have sub-categorized your parameters, the constructor becomes simpler, like:

def __init__(self, name, locations, times, types, md5, size, public_key=None, preview=None):

But, though you have simplified this class's constructor, you now have to build up a hierarchy of data to initialize it.... which is not simple.

Recommendation

As for me, given the nature of your JSON data, it appears that the large constructor is the lesser of the evils. It is 'tailored' to match the JSON data format, and it works well.

Your question:

My colleague says that the constructor is very large, and this code is horrible. Is he right?

The answer is: yes, it is large, and the code is ugly.... and he's right. But, not everything can be small, and pretty. This is the lesser of the evils.

I would not change it. Your class has a lot of related data, it has to get in there somehow.

Other Issues

Your constructor has the input public_key but does not store that as a property.

You should consider using a generator in your get_list_of_all_files: you should yield each File, instead of returning the completed list.

Answer 2

A quick-and-dirty solution is to just store the whole dictionary as-is. For convenience, define the __getattr__(...) method.

class File:
 def __init__(self, attributes):
 self.attributes = attributes
 def __getattr__(self, name):
 return self.attributes[name]

Note that Python itself uses basically the same strategy when storing instance members:

A class instance has a namespace implemented as a dictionary which is the first place in which attribute references are searched. When an attribute is not found there, and the instance’s class has an attribute by that name, the search continues with the class attributes. ... If no class attribute is found, and the object’s class has a __getattr__() method, that is called to satisfy the lookup.

---

A better solution would be to use meta-programming. This preserves the original functionality, including parameter validation and defaulting.

import inspect
class File:
 def __init__(self, name, created, modified, media_type, path, md5, type, mime_type, size,
 public_key=None, public_url=None, preview=None):
 args, _, _, values = inspect.getargvalues(inspect.currentframe())
 for arg in args[1:]:
 self.__dict__[arg] = values[arg]