SOLID - Is the principle of single responsibility applicable to methods in the class?

Question

I'm not sure if this method violates the principle of shared responsibility within my class,

public function save(Note $note) { if (!_id($note->getid())) { $note->setid(idGenerate('note')); $q = $this->db->insert($this->table) ->field('id', $note->getid(), 'id'); } else { $q = $this->db->update($this->table) ->where('AND', 'id', '=', $note->getid(), 'id'); } $q->field('title', $note->getTitle()) ->field('content', $note->getContent()); $this->db->execute($q); return $note; } 

Basically, he performs two tasks in the method - insert or update.

Should I divide it into two methods instead of following the principle of single responsibility?

But SRP is only for classes , right? Does this apply to methods inside a class?

SRP -

a class should have only one responsibility (i.e. only one potential change in software specification should affect the class specification)

EDIT:

Another way to list notes (including many different types of lists), search for notes, etc.

 public function getBy(array $params = array()) { $q = $this->db->select($this->table . ' n') ->field('title') ->field('content') ->field('creator', 'creator', 'id') ->field('created_on') ->field('updated_on'); if (isset($params['id'])) { if (!is_array($params['id'])) { $params['id'] = array($params['id']); } $q->where('AND', 'id', 'IN', $params['id'], 'id'); } if (isset($params['user_id'])) { if (!is_array($params['user_id'])) { $params['user_id'] = array($params['user_id']); } # Handling of type of list: created / received if (isset($params['type']) && $params['type'] == 'received') { $q ->join( 'inner', $this->table_share_link . ' s', 's.target_id = n.id AND s.target_type = \'note\'' ) ->join( 'inner', $this->table_share_link_permission . ' p', 'p.share_id = s.share_id' ) # Is it useful to know the permission assigned? ->field('p.permission') # We don't want get back own created note ->where('AND', 'n.creator', 'NOT IN', $params['user_id'], 'uuid'); ; $identity_id = $params['user_id']; # Handling of group sharing if (isset($params['user_group_id']) /*&& count($params['user_group_id'])*/) { if (!is_array($params['user_group_id'])) { $params['user_group_id'] = array($params['user_group_uuid']); } $identity_id = array_merge($identity_id, $params['user_group_id']); } $q->where('AND', 'p.identity_id', 'IN', $identity_id, 'id'); } else { $q->where('AND', 'n.creator', 'IN', $params['user_id'], 'id'); } } # If string search by title if (isset($params['find']) && $params['find']) { $q->where('AND', 'n.title', 'LIKE', '%' . $params['find'] . '%'); } # Handling of sorting if (isset($params['order'])) { if ($params['order'] == 'title') { $orderStr = 'n.title'; } else { $orderStr = 'n.updated_on'; } if ($params['order'] == 'title') { $orderStr = 'n.title'; } else { $orderStr = 'n.updated_on'; } $q->orderBy($orderStr); } else { // Default sorting $q->orderBy('n.updated_on DESC'); } if (isset($params['limit'])) { $q->limit($params['limit'], isset($params['offset']) ? $params['offset'] : 0); } $res = $this->db->execute($q); $notes = array(); while ($row = $res->fetchRow()) { $notes[$row->uuid] = $this->fromRow($row); } return $notes; } 

Answer 1

The method saves the note in the database. If this is what he was supposed to do, then one responsibility and implementation is in order. You will need to establish the logic for deciding whether to embed or update somewhere, this seems like a good place like anyone.

Only if you ever needed to explicitly do inserts or updates without the logic of an implicit solution, would it be advisable to separate the two into different methods that can be called separately. But at the moment, saving them in one and the same way simplifies the code (since the second half is shared), therefore, probably the best implementation.

Exempli gratia:

 public function save(Note $note) { if (..) { $this->insert($note); } else { $this->update($note); } } public function insert(Note $note) { .. } public function update(Note $note) { .. } 

The above would make sense if you sometimes had to call insert or update explicitly for some reason. SRP is not really the cause of this separation.

Answer 2

SOLID principles apply to terminology at the class level; they do not explicitly indicate methods. SRP itself states that classes should have one reason for change, so if you can replace responsibility that is wrapped in one class, you are fine.

Consider this:

 $userMapper = new Mapper\MySQL(); // or $userMapper = new Mapper\Mongo(); // or $userMapper = new Mapper\ArangoDb(); $userService = new UserService($userMapper); 

All these switches implement one interface and fulfill one responsibility - they are engaged in abstract access to storage for users. Therefore, mappers have one reason to change, as you can easily change them.

In your case, it's not about SRP. This is more about best practice. Best practice for methods is that they should only do one thing when possible, and take as few arguments as possible. This makes reading and finding errors easier.

There is another principle called the Least Surprise Principle . He simply states that method names must explicitly do what their names mean.

Returning to your code example:

save() implies that all this relates to saving data (updating an existing record), not creating it. By performing both insert and update, you are breaking PoLA.

So that when you explicitly call insert() , you know and expect it to add a new record. The same thing about the update() method - you know and expect that it will update the method, it will not create a new one.

Therefore, I will not do both things in save() . If I want to update a record, I would call update() . If I want to create a record, I would call insert() .