1
\$\begingroup\$

I noticed this old class of mine:

class SvgDrawing:

    def __init__(self, print_layout):
        self.print_layout = print_layout

        with open(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_start.txt'), mode='r') as f:
            self.svg_start = f.read()

        with open(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_end.txt'), mode='r') as f:
            self.svg_end = f.read()

And I thought I would refactor it like this to avoid re-reading the same file every time an instance is created.

class SvgDrawing:

    with open(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_start.txt'), mode='r') as f:
        svg_start = f.read()

    with open(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_end.txt'), mode='r') as f:
        svg_end = f.read()

    def __init__(self, print_layout):
        self.print_layout = print_layout

The new code works and improves the performance, but... something is telling me that there should be a better way.

Is it correct to put code that does something more complex than just assigning constant values in the class definition?

\$\endgroup\$
  • \$\begingroup\$ Here is a good read on the topic. \$\endgroup\$ – alecxe Feb 1 '17 at 19:20
2
\$\begingroup\$

Is it correct to put code that does something more complex than just assigning constant values in the class definition?

In general, I try to avoid it. Any code in the class definition will be run as soon as the class definition is executed – that is, as soon as somebody imports your code. So putting complex code in the class definition causes problems for importers:

  • If the code is slow, you slow down the rest of the program
  • If the code has side effects, you’re breaking the rule of least surprise – it’s quite unusual for importing a module to have side effects
  • If the code can throw exceptions (e.g. if you try to read svg_start.txt and it’s not there), same thing. When I import a library, the only exception I expect it might throw would be an ImportError. Throwing an OSError is generally quite unusual.

So I’d try to avoid it if you can. Only run that code when you need it – i.e., when somebody starts making instances of the class.


In this case, I’d think of maybe defining a descriptor, which caches the file contents, and then apply that to the class, like so:

class CachedFile:

    def __init__(self, path):
        self.path = path
        self.contents = None

    def __get__(self, instance, type):
        if self.contents is None:
            self.contents = open(self.path).read()
        return self.contents


class SVGDrawing:

    svg_start = CachedFile(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_start.txt'))
    svg_end = CachedFile(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_end.txt'))

    def __init__(self, *args, **kwargs):
        pass

Note that in this case, the file reads are deferred right up to the point where we need them – much more efficient than doing a read at import time. The flipside is that this code is a bit more complicated – you should decide if the performance gain is worth it.

\$\endgroup\$
  • \$\begingroup\$ Thanks, very helpful. Is there a way to do the same from __init__() without using the mutable list trick? \$\endgroup\$ – stenci Feb 4 '17 at 2:06
  • \$\begingroup\$ @stenci What mutable list trick? \$\endgroup\$ – alexwlchan Feb 4 '17 at 18:44
  • \$\begingroup\$ Adding a mutable class member like svg = [], then assigning the value in __init__ with svg[0] = txt. svg hasn't changed, so it is still visible as a class member, its content has changed. It works, but it's hacky. \$\endgroup\$ – stenci Feb 6 '17 at 13:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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