Best practices python classes

Question

I have some code that looks roughly like

class Foo:
    def __init__(self, a, b, c=None):
        self.bar = a
        if c:
            self.data = self.calc(b, c)
    def calc(self, bat, baz):
        """calculates some stuff that feels like should belong with the instance of `Foo`"""

1. Is it bad to have code within my __init__?

2. If I have a method that generates some data relating to the class instance, is it better practice to leave the user to call that method and store the data, or have one of the instance variables store it upon instantiation?

The reason for 2 is because I have this class where everything can be generated from some json file, so should the class just have one instance variable and a bunch of methods? Should this not even be a class? I always thought that classes with one instance variable were bad practice, but on the other hand, I have a group of functions that all operate on the same json file so they feel like they should belong in some conglomerate structure.

EDIT: As a third question, relating to 2, what if some of the methods require data that depends on the initialized data, and calculated data. So in the example above, suppose I (the user) calculate d=2*a+b because it is of interest to me. Then I want to do another computation that in theory would be sped up if I gave it d. So a method that looked like new_calc(self, d). Should I store d in the class when I calculate it initially, then not ask the user for it, or should I make them more responsible?

Answer 1

Is it bad to have code within my init?

No. But if it does anything complicated, factor out the code into a function/method, just like you did with calc. Only I'd suggest that you use _calc instead of calc if the only caller of that method is the constructor. That way, you signal to users of the class that they're not meant to call _calc themselves.

A nitpick:

if c:
   self.data = self.calc(b, c)

I'd be wary of this code, because if c is None (or 0, or False), self.data remains unset. So all the rest of your methods have to check whether the data attribute exists. It might be better to write

self.data = self.calc(b, c) if c is None else None

Or you could just move the check for None into calc method and have the line in the constructor say

self.data = self.calc(b, c)

is it better practice to leave the user to call that method

I'd think that depended on the exact use case. If each instance represented a specific data set, and there was no need to swap out one set of data for another one, I'd say it makes more sense for the constructor to do it. This way, the data is nicely encapsulated by the instance, and the user can't make a stupid mistake by changing the data when it isn't supposed to change.