SOLID - czy zasada jednej odpowiedzialności dotyczy metod w klasie?

Question

Nie jestem pewien, czy ta metoda w mojej klasie narusza zasada jednej odpowiedzialności,

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; 
} 

Zasadniczo robi dwa zadania w metodzie - Włożyć lub aktualizacji.

Czy powinienem oddzielić na dwie metody zamiast stosować się do zasady jednej odpowiedzialności?

Ale SRP jest przeznaczony tylko dla klas , czyż nie? Czy dotyczy to metod wewnątrz klasy?

SRP -

klasa powinna mieć tylko jeden odpowiedzialność (tj tylko jeden potencjalną zmianę specyfikacji Oprogramowania powinny móc wpływać na specyfikację klasy)

EDYCJA:

Inna metoda notowania notatek (w tym wiele różnych rodzajów ofert), wyszukiwanie n Uwagi, 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

Sposób utrzymuje notatkę do bazy. Jeśli tak właśnie powinno być, to jest to jedna odpowiedzialność, a wdrożenie jest w porządku. Musisz podać logikę, decydując, czy wstawić lub zaktualizować gdzieś na, to wydaje się równie dobre miejsce jak każde inne.

Tylko wtedy, gdy konieczne jest jawne wprowadzanie wstawek lub aktualizacji bez domyślnej logiki decyzyjnej, warto byłoby rozdzielić te dwie metody na różne metody, które można wywoływać osobno. Ale w tej chwili zachowanie ich w tej samej metodzie upraszcza kod (ponieważ druga połowa jest udostępniana), więc jest to prawdopodobnie najlepsza implementacja.

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) { 
    .. 
} 

Powyższe miałoby sensu, jeśli czasami potrzebne zadzwonić insert lub update wyraźnie z jakiegoś powodu. SRP tak naprawdę nie jest powodem tego rozstania.

Answer 2

Zasady SOLID stosowane są do terminologii na poziomie klasy, nie mówią jednoznacznie o metodach. Sam SRP stwierdza, że ​​klasy powinny mieć jeden powód do zmiany, więc dopóki można zastąpić odpowiedzialność, która jest opakowana w jedną klasę, jesteś w porządku.

Rozważ to:

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

$userService = new UserService($userMapper); 

Wszystkie te elementy odwzorowujące realizować jeden interfejs i służyć jednej odpowiedzialność - robią abstrakcyjne dostępu do przechowywania użytkowników. Dlatego też twórcy map mają jeden powód do zmiany, ponieważ można je łatwo zamienić.

Twoja sprawa nie dotyczy ogólnie SRP. Chodzi bardziej o najlepszą praktykę.Cóż, najlepsza praktyka dotycząca metod stwierdza, że ​​powinni oni robić tylko jedną rzecz, gdy tylko jest to możliwe i akceptować jako mniej argumentów, jak to tylko możliwe. To ułatwia czytanie i znajdowanie błędów.

Jest jeszcze jedna zasada, która nazywa Principle of Least Astonishment. Po prostu stwierdza, że ​​nazwy metod powinny jawnie robić to, co sugerują ich nazwy.

Schodząc do przykładu kodu:

save() sugeruje, że to wszystko o zapisywanie danych (aktualizowania istniejącego rekordu), nie tworząc. Robiąc zarówno wstawianie, jak i aktualizowanie, łamiesz PoLA.

To jest to, gdy dzwonisz explicite insert() znasz i oczekiwać, że będzie dodać nowy rekord. Podobnie jest z metodą update() - wiesz i oczekujesz, że zaktualizuje ona metodę, nie utworzy nowej.

Dlatego nie będę robić obie rzeczy w save(). Jeśli chcę zaktualizować rekord, zadzwoniłbym pod numer update(). Jeśli chcę utworzyć rekord, zadzwoniłbym pod numer insert().