ohey.pl

Niedawno w ofercie wydawnictwa helion pojawiła się ciekawa książka - Czyty Kod (ang. "Clean code", autor Robert C. Martin). Książka ta opisuje techniki oraz zasady pisania czystego, czytelnego oraz modyfikowalnego kodu. Kilka zasad z tej książki w mojej opinii jest dosyć kontrowersyjnych, ale książka sama w sobie porusza bardzo ważną kwestię, a przede wszystkim jest ona naprawdę dobra.

Głównymi przesłaniami tej książki to:

• waga nazewnictwa klas, funkcji, zmiennych składowych, zmiennych lokalnych i wszystkich pozostałych bytów (pakietów, przestrzeni nazw itp.)


• wszystkie byty (klasy, funkcje, struktury danych itp.) powinny być maksymalnie proste i krótkie. Powinny przedstawiać jedną rzecz, realizować jedną czynność, której istota jest zawarta w opisowej nazwie (punkt pierwszy)

Od tych dwóch zaleceń wywodzi się szereg innych dotyczących zasad budowania klas, funkcji, zależności między modułami i wiele innych. Nie będę ich tutaj oczywiście przytaczał, zainteresowanych odsyłam do tej pozycji.

Idąc śladami autora tej książki, przebuduję mój kod który już wcześniej zamieściłem w notce "Wielostronicowe formularze". Napisałem testy jednostkowe do tej klasy (pisząc plugin, testów nie napisałem ;]), po każdej wprowadzonej zmianie odpalałem testy aby sprawdzić czy klasa nadal działa tak jak powinna.

Nazewnictwo

Pierwszą rzeczą którą zrobiłem to zmiany nazw niektórych metod oraz składowych:
- zmiana get/setPage() na get/setCurrentPageNumber, validatePage na validatePageNumber
- zmiana getPages() na getNumberOfForms()
- zmiana składowej globalNamespace na useGlobalNamespace, analogicznie zmiana nazwy metod dostępowych

Zawahałem się przy metodzie setForms, okazało się że nazwa tej metody wprowadza w błąd, gdyż nie ustawia ona formularzy, a jedynie nadpisuje formularze dla pierwszych stron (błąd logiczny - jeśli przekazaliśmy mniejszą liczbę formularzy niż było ich wcześniej, to na końcu tablicy zostaną formularze z przed wywołania metody). Zdecydowałem się na napisanie dodatkowej funkcji o nazwie addForms, która dopisuje tablicę formularzy. Metoda setForms, tak jak nazwa wskazuje, nadpisuje wcześniej dodane formularze.

Wprowadziłem również inne drobne zmiany nazw (np. zmiennych lokalnych) i wydają się już one być dobre, teraz czas zająć się metodami.

Metody / funkcje

Najważniejsze jest to aby metoda operowała na jednym poziomie abstrakcji, realizowała jedną czynność i była maksymalnie krótka. Na pierwszy rzut oka większość metod spełnia te warunki, ale jest kilka które należałoby poprawić.

Na pierwszy ogień poszła metoda addForm.

[PHP]
public function addForm(sfForm $form)
  {
    $index = count($this->forms);
    $form->setOption('page', $index+1);
    $this->forms[$index] = $form;
 
    if(!$this->isUseGlobalNamespace() && $key = $this->getFormKeyName($form, true))
    {
      $format = $form->getWidgetSchema()->getNameFormat();
      $format = sprintf($this->getNameFormat(), substr($format, 0, $k = strpos($format, '['))).substr($format, $k);
      $form->getWidgetSchema()->setNameFormat($format);
    }
    else
    {
      $form->getWidgetSchema()->setNameFormat($this->getNameFormat());
    }
  }

Robi ona co najmniej trzy rzeczy: oblicza indeks formularza oraz kolejny numer strony, dodaje formularz do tablicy oraz zmienia nazwę formatu widgetu. Nie operuje ona również na jednym poziomie abstrakcji, gdyż przykładowo wyznaczanie numeru indeksu oraz numeru strony jest zadaniem niższego poziomu niż samo dodawanie formularza do tablicy. Rozbiłem tą metodę na kilka mniejszych, odpowiedzialnych za jedną czynność. Wyodrębniłem metodę wykonującą dodawanie formularza do tablicy na odpowiednim miejscu, konwertującą indeks formularza do numeru strony, czy też zmieniającą nazwę formatu formularza.

[PHP]
public function addForm(sfForm $form)
  {
    $this->doAddForm($form);
    $this->setFormNameFormat($form);
  }
 
  private function doAddForm(sfForm $form)
  {
    $index = $this->getNumberOfForms();
    $form->setOption('page', $this->convertIndexToPageNumber($index));
    $this->forms[$index] = $form;
  }
 
  private function convertIndexToPageNumber($index)
  {
    return ($index+1);
  }
 
  private function setFormNameFormat(sfForm $form)
  {
    if($this->shouldBeUsedGlobalNamespace($form))
    {
      $form->getWidgetSchema()->setNameFormat($this->getNameFormat());
    }
    else
    {
      $format = $form->getWidgetSchema()->getNameFormat();
      $format = sprintf($this->getNameFormat(), substr($format, 0, $k = strpos($format, '['))).substr($format, $k);
      $form->getWidgetSchema()->setNameFormat($format);
    }
  }

Jak widać kod jest nieco dłuższy, ale głównie ze względu na sposób formatowania kodu. Na tej operacji zyskaliśmy za to czytelność oraz wyodrębnienie przydatnej metody konwertującej indeks do numeru strony.

Z metody getForm wyodrębniłem metodę konwertującą numer strony do indeksu formularza, więc zyskaliśmy na spójności mając dwie metody konwertujące te wartości między sobą. W miejscach gdzie taka konwersja była dokonywana wykorzystałem te metody.

Kolejną metodą, którą trzeba wyremontować jest metoda bind. Jest to najdłuższa metoda całej klasy.

Obecnie metoda ta wykonuje następujące czynności: przypisuje parametry do składowych klasy, resetuje stan formularza (wartości, flagę isBound, tworzy nowy obiekt przechowujący błędy), binduje kolejne formularze zapisując przy okazji ewentualne błędy, tworzy tablicę wartości formularza.

Wyodrębniłem metodę resetFormState, wyodrębniłem metodę dodającą wartości z formularza składowego do głównego formularza stronicowanego (metoda addValuesFromForm) oraz bindującą jeden formularz składowy.

Metoda getFormKeyName tworząca klucz dla formularza wg formatu nazwy widgetu również była zbyt duża, ilustruje to poniższy listing.

[PHP]
private function getFormKeyName(sfForm $form, $first = false)
  {
    if(isset($this->formsKeys[$form->getOption('page')]) && !$first)
    {
      return $this->formsKeys[$form->getOption('page')];
    }
 
    $format = $form->getWidgetSchema()->getNameFormat();
    if($first)
    {
      return substr($format, 0, strpos($format, '['));
    }
 
    $format = substr($format, strpos($this->getNameFormat(), '['));
 
    if($c = strpos($format, '[%s]'))
    {
      $key = trim(substr($format, 0, $c), '[]');
    }
    else
    {
      $key = false;
    }
 
    return $key;
  }

Zmieniłem w niej warunek początkowy tak aby wartość zwracana była w jednym miejscu oraz wyodrębniłem metodę buildFormKeyName. Drugi parametr był tak zwanym parametrem znacznikowym, zmieniał on działanie metody. Istnienie takiego parametru sugeruje nam podział metody na dwie mniejsze, co zresztą uczyniłem.

[PHP]
private function getFormKeyName(sfForm $form)
  {
    $formIndex = $form->getOption('page');
    if(!isset($this->formsKeys[$formIndex]))
    {
      $format = $form->getWidgetSchema()->getNameFormat();
      $this->formsKeys[$formIndex] = $this->buildFormKeyName($format);
    }
 
    return $this->formsKeys[$formIndex];
  }
 
  private function buildFormKeyName($format)
  {
    $format = substr($format, strpos($this->getNameFormat(), '['));
 
    $c = strpos($format, '[%s]');
    $key = $c ? trim(substr($format, 0, $c), '[]') : false;
 
    return $key;
  }
 
  private function hasNameFormat(sfForm $form)
  {
    $format = $form->getWidgetSchema()->getNameFormat();
 
    return ((bool) substr($format, 0, strpos($format, '[')));
  }

Ostatnia metoda która wymaga przebudowy to getValuesForForm. Pierwsze co rzuciło mi się w oczy to warunek w pierwszej instrukcji warunkowej - już go gdzieś widziałem... Trzymając się zasady DRY, kod z instrukcji warunkowej umieściłem w nowo utworzonej metodzie. Wyodrębniłem również osobną metodę do budowania tablicy wartości formularza z określonej strony gdy jest używana wspólna "przestrzeń nazw" (dane są przechowywane jako tablica jednowymiarowa) dla wszystkich formularzy.

Zmieniłem również specyfikatory dostępu przy składowych z chronionych na prywatne aby zwiększyć hermetyzację danych. Operacje na składowych w podklasach powinny się odbywać tylko i wyłącznie poprzez metody dostępowe. Każda metoda, która powstała na skutek przebudowy innej jest prywatna.

Komentarze

Kolejną ważną kwestią są komentarze, pisząc tą klasę starałem się ją dosyć dobrze opisać, wręcz przesadziłem z tym. Jest sporo nadmiarowości w tej kwestii, a więc kilka komentarzy należy wyrzucić. W php nie ma twardego typowania, więc dla wygody zostawię komentarze dokumentujące, które zawierają typ parametrów, składowych, wartości zwracanych, aby w IDE działało podpowiadanie składni.

Komentarze dosyć szybko tracą na ważności, przykładowo metoda getPages miała komentarz "Zwraca ilość stron" (swoją drogą nadmiarowy/głupi - niepotrzebne skreślić ;P). Obecnie metoda nazywa się getNumberOfForms, więc komentarz ma się nijak do nazwy oraz wykonywanej czynności przez tą metodę. Należy go bezwłocznie wyrzucić, aby nie wprowadzał w błąd. To jest bardzo poważny minus komentarzy, dlatego powinno się je pisać tylko w sytuacjach gdy są niezbędne, a kod sam siebie niezbyt jasno komentuje. Jeśli uważamy że dany fragment kodu nie jest zbyt jasny, to przed umieszczeniem komentarza należy się zastanowić czy nie lepiej napisać kod bardziej czytelnie, tak aby komentarz był zbędny.

Ostatnią kwestią którą się zajmę to kolejność metod w pliku źródłowym. Kod źródłowy w miarę możliwości powinien dać się czytać z góry do dołu. Każda metoda prywatna powinna być bezpośrednio za metodą, która ją po raz pierwszy wywołuje - tak też uczyniłem.

Podsumowanie

Poniżej załączam kod wyjściowej oraz oczyszczonej klasy wraz z testami jednostkowymi dla tej drugiej klasy (zmieniło się lekko api więc i testy uległy zmianie). Otrzymany kod jest krótszy o kilka wierszy, głównie za sprawą usunięcia nadmiarowych komentarzy. Oczywiście jest możliwy dalszy refaktoring, kod rzadko kiedy jest idealny. Znacznie więcej na temat czystego kodu znajdziecie w książce o tym tytule, polecam.

Kod źródłowy: pobierz