I am not sure whether this method inside my class is violating Single responsibility principle,
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 it does two jobs in a method - insert or update.
Should I separate it into two methods instead to comply with Single responsibility principle?
But SRP is meant for classes only, isn't it? Does it apply to the methods inside a class?
SRP -
a class should have only a single responsibility (i.e. only one potential change in the software's specification should be able to affect the specification of the class)
EDIT:
Another method for listing notes (including many different type of listings), searching 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;
}
The Single Responsibility Principle applies to the software that we develop on different levels: methods, classes, modules, and services (collectively, I'll call all these things components later in this article). So, the SRP states that each component should have a single responsibility.
The Single Responsibility Principle (SRP) states that a function or a class should have one responsibility. Following this principle will help keep your functions small and manageable.
The Single Responsibility Principle (SRP) The idea behind the SRP is that every class, module, or function in a program should have one responsibility/purpose in a program. As a commonly used definition, "every class should have only one reason to change". The class above violates the single responsibility principle.
The reason it is important to keep a class focused on a single concern is that it makes the class more robust. Continuing with the foregoing example, if there is a change to the report compilation process, there is a greater danger that the printing code will break if it is part of the same class.
The method persists the note to the database. If that's what it's supposed to do, then that's a single responsibility and the implementation is fine. You'll need to put the logic of deciding whether to insert or update somewhere, this seems as good a place as any.
Only if you ever needed to explicitly do inserts or updates without the implicit decision logic would it be worthwhile to separate those two out into different methods which can be called separately. But at the moment, keeping them in the same method simplifies the code (since the latter half is shared), so that's likely 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 needed to call insert
or update
explicitly for whatever reason. SRP is not really a reason for this separation though.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With