Is a method named createOrUpdate() violating the single responsibility principle? Should I split into create() and update()?

Question

I work developing software, and very often I find inconsistency in the company's code. The thing is, there is a method named createOrUpdate(), and this method basically does what its name says, creates or updates a given record. However, I believe this method violates the Single Responsibility Principle, because it does two things, there should be a method for every operation performed on the database.

However, I may be wrong about this. So, I would like to know the opinion of you who understand more about the subject.

Answer 1

It might sound like “create or update” are two different things. But in reality, these two concerns are often deeply intertwined and cannot be viewed separately.

Sometimes an implementation of such a function would indeed just delegate to create() and update() functions as necessary:

# version A
def create_or_update(data):
  try:
    create(data)
  except AlreadyExists:
    update(data)

# version B
def create_or_update(data)(;
  try:
    update(data)
  except NotFound:
    create(data)

But as soon as these functions imply communication with external systems, or as soon as the write should happen atomically, then it would be undesirable or even incorrect to separate them like that.

For example, the above code snippets are incorrect if create-or-update must happen atomically. Version A will fail if the record is deleted between the attempted creation and the attempted update. Version B will fail if a conflicting record is created in between the attempted update and the attempted creation. In some cases, such time-of-check to time-of-use (TOC/TOU) race conditions can even lead to severe security issues.

Thus, it is extremely common to have first-class support for such create-or-update operations. Related concepts are UPSERT/MERGE in databases or compare-and-swap instructions as synchronization primitives.

Some business problems are elegantly described only in terms of create-or-update, without separate creation or update functionality. This is the case in particular when we want to bring a resource into a particular state. For example, uploading a file with a particular file name might create a new file if it doesn't exist, or overwrite the old state. See also the HTTP PUT method.

In some specific cases, concurrent modifications can also be resolved via conflict-free replicated datatypes (CRDTs). A completely different approach is to only describe change events to a state without directly updating a state, an approach that is useful in event-based architectures.

Answer 2

Is a method named createOrUpdate() violating the single responsibility principle?

No.

Why? Because:

Robert C. Martin, the originator of the term, expresses the principle as, "A class should have only one reason to change,"

Wikipedia - Single Responsibility Principle

And, like you said, this is a method. Not a class.

Maybe you're thinking of "do one thing". As in:

Functions should do one thing. They should do it well. They should do it only.

Clean Architecture - Robert Martin p35

The problem is, every function does more than one thing. So what does this rule even mean? It means everything that does and doesn't belong in the function must be defined by one thing. It means there should be some single thing that covers everything the function does. Anyone who knows that thing should not be surprised by what they find in the function. ^{See also PoLA}

One Level of Abstraction per Function

In order to make sure our functions are doing “one thing,” we need to make sure that the statements within our function are all at the same level of abstraction.

Clean Architecture - Robert Martin p36

Note the word "statements" is plural. As in, you can have more than one statement in the function that only does "one thing". Therefor, "one thing" doesn't mean that the function can't be decomposed. It can both add and subtract. It can both create or update. That's fine. We just want some "one thing" that makes clear what those things are.

What you really need is a better name.

Let’s assume the point of your method named createOrUpdate() is it's post condition: something exists. You could chose to name it, not after the statements it contains, but after the point of calling it. After it has been successfully called something has been ensured to exist. So a better name might be ensureExists().

Named that way you're free to refactor it so it contains different statements like write or new. By keeping the statements out of the name we've hidden implementation details. Done this way, no promise has been made about how the method works. Just what you can count on it doing.

This puts the methods name at a higher level of abstraction. It's still one thing. createOrUpdate() was also one thing, just didn't look like it. The improvement here is that now the name is focused on what the caller cares about. Not what the implementation is doing.

That's the real reason to rename this. I don't want to be surprised by what I find inside. But I don't want to find the insides leaking into the name.

Answer 3

Yours is a very unfortunate example. Separate check for existence and creation is a major security risk. It may go wrong if one thread creates a resource just after another thread checked that it doesn’t exist - in reality such a check cannot return “this resource doesn’t exist” but only “this resource didn’t exist when I looked”. So things can go wrong naturally and if things can go wrong in normal operation of your software, you can bet someone will find a way to exploit this.