Should a modifying class method save itself or be explicity called after the method is called?

Question

Suppose a class has a method that modifies it's internals. Should that method call save on itself before returning or should the save be left to the caller to explicitly save after the modifying method has been called?

Example:

Explicitly calling save:

class Bar(models.Model):
    def set_foo(self, foo):
        self.foo = foo

bar = Bar()
bar.set_foo("foobar")
bar.save()

or allowing method to call save:

class Bar(models.Model):
    def set_foo(self, foo):
        self.foo = foo
        self.save()

bar = Bar()
bar.set_foo("foobar")

I'm working with django, but I was wondering if there was a best practice in django or in general for this situation.

Answer 1

The user of your API might want to make several changes, saving the object after every change is anything but good so no, don't call save in your method.

Answer 2

The user of your API might forget to call .save() and then get screwed. So I think its better to call save for him. For cases like those Daslch mentions, if it makes sense, you can define:

def set_foo(self, foo, skip_save=False):
    self.foo = foo
    if not skip_save:
        self.save()

so the user can, if she wishes to (and explicitly states that), avoid the save.

Answer 3

Actually, I agree with both Ofri and Daslch ... depending on what day of the week it is. If this is just one of many modification routines you might do to a particular object, then it will get quite expensive having each of them do their own save. On the other hand, if this is a rare, self-contained event then you want to do the save because it may not be obvious to the caller (ie, someone other than you that it needs to be done.

For example, tagging events (which use ManyToMany anyway) should require no additional save() on the programmers part.

Answer 4

To deal with all the issues expressed in various existing answers, I suggest the following approach: make a method, call it say saving or modifying, that is a context manager. The entry to that method sets a private flag which says the modification is in progress; the exit resets the flags and performs the saving; all modifying methods check the flag and raise an exception if not set. For example, using a base class and a save method that real subclasses must override:

import contextlib

class CarefullyDesigned(object):

    def __init__(self):
      self.__saving = False

    def _save(self):
      raise NotImplementedError('Must override `_save`!')

    def _checksaving(self):
      "Call at start of subclass `save` and modifying-methods"
      if not self.__saving: raise ValueError('No saving in progress!')

    @contextlib.contextmanager
    def saving(self):
      if self.__saving: raise ValueError('Saving already in progress!')
      self.__saving = True
      yield
      self._save()
      self.__saving = False

Example use...:

class Bar(models.Model, CarefullyDesigned):

    def __init__(self, *a, **k):
        models.Model.__init__(self, *a, **k)
        CarefullyDesigned.__init__(self)

    def _save(self):
        self._checksaving()
        self.save()

    def set_foo(self, foo):
        self._checksaving()
        self.foo = foo

    def set_fie(self, fie):
        self._checksaving()
        self.fie = fie

bar = Bar()
with bar.saving():
    bar.set_foo("foobar")
    bar.set_fie("fo fum")

This guarantees the user won't forget to call saving nor accidentally call it in a nested way (that's the purpose of all of those exceptions), and call save only once when the group of modifying-methods is done, in a handy and, I'd say, pretty natural way.