Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

SOLID - does Single responsibility principle apply to methods in a class?

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;
}
like image 796
Run Avatar asked Jun 16 '15 07:06

Run


People also ask

Does SRP apply to methods?

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.

Does Single Responsibility Principle apply to functions?

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.

What is the Single Responsibility Principle and how does it apply while writing functions?

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.

Why should a class focus on a single responsibility?

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.


1 Answers

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.

like image 57
deceze Avatar answered Sep 22 '22 11:09

deceze