37

I have a method that updates employees data in the database. The Employee class is immutable, so "updating" the object means actually to instantiate a new object.

I want the Update method to return a new instance of Employee with the updated data, but since now I can say the responsibility of the method is to update the employee data, and fetch from the database a new Employee object, does it violate the Single Responsibility Principle?

The DB record itself is updated. Then, a new object is instantiated to represent this record.

  • 5
    A little pseudo code could go a long way here: Is there actually a new DB record created, or is the DB record itself updated, but in the "client" code a new object is created because the class is modelled as immutable? – Martin Ba Jul 5 '16 at 8:56
  • Additionally to what Martin Bra asked, why are you returning a Employee instance when you're updating the database? The values of Employee instance have not changed in the Update method so why return it (callers already have access to the instance...). Or does the update method also retrieves (potentially different) values from the database? – Thorsal Jul 5 '16 at 9:52
  • 1
    @Thorsal : if your data entities are immutable, returning an instance with updated data is pretty much SOP, since otherwise you'd have to do the instantiation of the modified data yourself. – mikołak Jul 5 '16 at 9:59
  • 21
    Compare-and-swap and test-and-set are fundamental operations in the theory of multi-threaded programming. They both are update methods with a return type, and could not work otherwise. Does this break concepts like command-query separation or Single Responsibility Principle? Yes, and that is the whole point. SRP is not a universally good thing, and can in fact be actively harmful. – MSalters Jul 5 '16 at 11:43
  • 3
    @MSalters: Exactly. Command/query separation says it should be possible to issue queries which can be recognized as idempotent, and issue commands without having to wait for a reply, but atomic read-modify-write should be recognized as a third category of operation. – supercat Jul 5 '16 at 14:11
16

As with any rule, I think the important thing here is to consider the purpose of the rule, the spirit, and not get mired in analyzing exactly how the rule was worded in some textbook and how to apply that to this case. We don't need to approach this like lawyers. The purpose of the rules is to help us write better programs. It's not like the purpose of writing programs is to uphold the rules.

The purpose of the single-responsibility rule is to make programs easier to understand and maintain by making each function do one self-contained, coherent thing.

For example, I once wrote a function that I called something like "checkOrderStatus", that determined if an order was pending, shipped, back-ordered, whatever, and returned a code indicating which. Then another programmer came along and modified this function to also update the quantity on hand when the order was shipped. This severely violated the single responsibility principle. Another programmer reading that code later would see the function name, see how the return value was used, and might well never suspect that it did a database update. Someone who needed to get the order status without updating the quantity on hand would be in an awkward position: should he write a new function that duplicates the order status part? Add a flag to tell it whether to do the db update? Etc. (Of course the right answer would be to break the function in two, but that might not be practical for many reasons.)

On the other hand, I wouldn't nitpick what constitutes "two things". I just recently wrote a function that sends customer information from our system to our client's system. That function does some reformatting of the data to meet their requirements. For example, we have some fields that may be null on our database, but they don't allow nulls so we have to fill in some dummy text, "not specified" or I forget the exact words. Arguably this function is doing two things: reformat the data AND send it. But I very deliberately put this in a single function rather than having "reformat" and "send" because I don't want to ever, ever send without reformatting. I don't want someone to write a new call and not realize he has to call reformat and then send.

In your case, update the database and return an image of the record written seem like two things that might well go together logically and inevitably. I don't know the details of your application so I can't say definitively if this is a good idea or not, but it sounds plausible.

If you are creating an object in memory that holds all the data for the record, doing the database calls to write this, and then returning the object, this makes a lot of sense. You have the object in your hands. Why not just hand it back? If you didn't return the object, how would the caller get it? Would he have to read the database to get the object you just wrote? That seems rather inefficient. How would he find the record? Do you know the primary key? If someone declares that it's "legal" for the write function to return the primary key so that you can re-read the record, why not just return the whole record so you don't have to? What's the difference?

On the other hand, if creating the object is a bunch of work quite distinct from writing the database record, and a caller might well want to do the write but not create the object, then this could be wasteful. If a caller might want the object but not do the write, then you'd have to provide another way to get the object, which could mean writing redundant code.

But I think scenario 1 is more likely, so I'd say, probably no problem.

  • Thanks for all the answers, but this one really helped me the most. – Sipo Jul 7 '16 at 7:21
  • If you don't want to send without reformatting, and there is no use for reformatting beside sending the data later, then they're one thing, not two. – Joker_vD Jul 7 '16 at 8:21
  • public function sendDataInClientFormat() { formatDataForClient(); sendDataToClient(); } private function formatDataForClient() {...} private function sendDataToClient() {...} – CJ Dennis Jul 11 '16 at 0:08
  • @CJDennis Sure. And in fact that's how I did it: A function to do the formatting, a function to do the actual sending, and there were two other functions that I won't get into here. Then one top-level function to call them all in proper sequence. You could say "format and send" is one logical operation and thus properly could be combined in one function. If you insist that it's two, ok, then still the rational thing to do is have one top-level function that does it all. – Jay Jul 12 '16 at 13:02
67

The concept of the SRP is to stop modules doing 2 different things when doing them individually makes for better maintenance and less spaghetti in time. As the SRP says "one reason to change".

In your case, if you had a routine that "updates and returns the updated object", you are still changing the object once - giving it 1 reason to change. That you return the object right back is neither here nor there, you're still operating on that single object. The code has responsibility for one, and only one, thing.

The SRP is not really about trying to reduce all operations to a single call, but to reduce what you're operating on and how you're operating on it. So a single update that returns the updated object is fine.

  • 7
    Alternatively, it's not "about trying to reduce all operations to a single call," but to reduce how many different cases you have to think about when using the item in question. – jpmc26 Jul 5 '16 at 14:43
46

As always, this is a question of degree. The SRP should stop you from writing a method that retrieves a record from an external database, performs a fast Fourier transform on it and updates a global statistics registry with the result. I think almost everyone would agree these things should be done by different methods. Postulating a single responsibility for each method is simply the most economical and memorable way to make that point.

At the other end of the spectrum are methods that yield information about the state of an object. The typical isActive has giving this information as its single responsibility. Probably everyone agrees that this is okay.

Now, some extend the principle so far that they consider returning a success flag a different responsibility from performing the action whose success is being reported. Under an extremely strict interpretation, this is true, but since the alternative would be having to call a second method to obtain the success status, which complicates the caller, many programmers are perfectly fine with returning success codes from a method with side effects.

Returning the new object is one step further on the road to multiple responsibilities. Requiring the caller to make a second call for an entire object is slightly more reasonable than requiring a second call just to see whether the first one succeeded or not. Still, many programmers would consider returning the result of an update perfectly fine. While this can be construed as two slightly different responsibilities, it is certainly not one of the egregious abuses that inspired the principle to begin with.

  • 15
    If you have to call a second method to see if the first one succeeded, shouldn't you have to call a third method to get the result of the second one? Then if you actually want the result, well... – user186205 Jul 5 '16 at 18:26
  • 3
    I may add that over-zealous application of the SRP leads to a myriad of small classes, which is a burden in itself. How big a burden depends on your environment, compiler, IDE/helper tools etc. – Erik Alapää Jul 6 '16 at 6:56
8

Does it violate the Single Responsibility Principle?

Not necessarily. If anything, this violates the principal of command-query separation.

The responsibility is

update the employee data

with the implicit understanding that this responsibility includes the status of the operation; e.g, if the operation fails, an exception is thrown, if it succeeds, the update employee is returned, etc.

Again, this is all a matter of degree and subjective judgement.


So what about command-query separation?

Well, that principle exists, but returning the result of an update is in fact very common.

(1) Java's Set#add(E) adds the element and returns its previous inclusion in the set.

if (visited.add(nodeToVisit)) {
    // perform operation once
}

This is more efficient than the CQS alternative, which may have to perform two lookups.

if (!visited.contains(nodeToVisit)) {
    visited.add(nodeToVisit);
    // perform operation once
}

(2) Compare-and-swap, fetch-and-add, and test-and-set are common primitives that allow concurrent programming to exist. These patterns appear often, from low-level CPU instructions to high-level concurrent collections.

2

The single responsibility is about a class not needing to change for more than one reason.

As an example an employee has a list of phone numbers. When the way you handle phone numbers change (you might add country calling codes) that should not change the employee class at all.

I wouldn't have the employee class needing to know how it saves itself to the database, because it would then change for changes in employees and changes in how data is stored.

Similar there shouldn't be a CalculateSalary method in the employee class. A salary property is ok, but the calculation of tax etc. should be done somewhere else.

But that the Update method returns what it just have updated is fine.

2

Wrt. the specific case:

Employee Update(Employee, name, password, etc) (actually using a Builder since I have a lot of parameters).

It seems the Update Method takes an existing Employee as first parameter to identify(?) the existing employee and a set of parameters to change on this employee.

I do think this could be done cleaner. I see two cases:

(a) Employee does actually contain a database/unique ID by which it can always be identified in the database. (That is, you do not need the whole record values set to find it in the DB.

In this case, I would prefer a void Update(Employee obj) method, that just finds the existing record by ID and then updates the fields from the passed object. Or maybe an void Update(ID, EmployeeBuilder values)

A variation of this that I found useful is to only have a void Put(Employee obj) method that inserts or updates, depending on whether the record (by ID) exists.

(b) The full existing record is needed for DB lookup, in that case it might still make more sense to have: void Update(Employee existing, Employee newData).

As far as I can see here, I would indeed say that the responsibility of building a new object (or sub-object values) to store and actually storing it are orthogonal, so I would separate them.

The mentioned concurrent requirements in other answers (atomic set-and-retrieve / compare-swap, etc.) have not been an issue in the DB code I have worked on so far. When talking to a DB, I think this should be handled on the transaction level normally, not on the individual statement level. (That's not to say that there might not be a design where am "atomic" Employee[existing data] Update(ID, Employee newData) couldn't make sense, but with DB accesses it not something I normally see.)

1

So far everyone here is talking about classes. But think about it from an interface perspective.

If an interface method declares a return type, and/or an implicit promise about the return value, then every implementation needs to return the updated object.

So you can ask:

  • Can you think of possible implementations that don't want to bother returning a new object?
  • Can you think of components that depend on the interface (by having an instance injected), which do not need the updated employee as a return value?

Also think about mock objects for unit tests. Obviously it will be easier to mock without the return value. And dependent components are easier to test, if their injected dependencies have simpler interfaces.

Based on these considerations you can make a decision.

And if you regret it later, you could still introduce a second interface, with adapters between the two. Of course such additional interfaces increase the compositional complexity, so it is all a trade-off.

0

Your approach is fine. Immutability is a strong assertion. The only thing I would ask is: Is there any other place where you construct the object. If your object was not immutable you would have to answer additional questions because "State" is introduced. And the state change of an object may occur on different reasons. Then you should know your cases and they should not be redundant or divided.

protected by gnat Jul 13 '16 at 17:50

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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