Pierwsze próby refactoringu

Odpowiedz Nowy wątek
2016-02-17 19:38
0

Jestem w projekcie, który ma ok. 6-8 lat, a programuję ~ 7 msc. Staram się w miarę pozyskiwanej wiedzy (a tej przewija się ogrom) poprawiać co się da, żeby trochę ułatwić sobie życie. Jak się przekonałem nie zawsze wychodzi na plus, bo czasami przekombinowuje, albo stosuję się do kiepskich rad.

Kilka tygodni temu dostałem zadanie, w którym z grubsza chodziło o to, że nie zgadzały się liczby w raporcie. Był to dosyć spory raport, ponieważ brała w nim udział chyba tona tabel. Jak wszedłem do modelu to się mocno zdziwiłem, bo moim oczom ukazało się mniej więcej coś takiego:

public function getReport() {
    $result = $this->db->query(
    // 300 linii niczym nie zmąconego beztroskiego zapytania
    );
    return $result;
}

W kontrolerze oczywiście było kolejne 150 linii parsowania inputu tej cudownej funkcji, bo potrzebne było różne pogrupowanie wyników. Ponieważ mamy nieścisłości w bazie, to niektóre kolumny reprezentujące datę utworzenia nazywają się na 10 różnych sposobów, więc to było kolejne 20 linii dodatkowego kodu, przy czym mi się udało to trochę zwinąć.

Spróbowałem coś z tym zrobić. Efektem tego jest klasa, która w sumie jest wielkości tego jednego zapytania, ale przeniosłem również tam logikę z kontrolera i trochę ją upiększyłem.

Proszę Was o opinię na temat tego kawałka kodu. Z góry dziękuję wytrwałym :)

  • code.txt (0,01 MB) - ściągnięć: 127
edytowany 3x, ostatnio: Desu, 2016-02-17 19:41
W kwestii refactoringu zobacz książkę "Praca z zastanym kodem...", sporo podstaw ale dużo też ciekawych porad. - eL 2016-02-17 19:40
Właśnie niedawno przyszła, bo póki co jestem w połowie Refactoring: Improving the Design of Existing Code. Dzięki za pozycję :) - Desu 2016-02-17 19:43

Pozostało 580 znaków

2016-02-17 20:19
1

ta klasa i tak za dużo robi. Można spokojnie wszystkie te metody, które mają w nazwie Contact np getContactFinalDeedQuery czy getContactDeveloperAgreementQuery wydzielić do osobnej klasy. Kod klasy się skróci, a i wtedy nazwy tych metod nie będą musiały być tak długie.
Nazwa metody getTotals mało mówi, co zwraca.
Linia #119 - ZZZZZ - ktoś zasnął nad kodem? :D
Kod w linijkach 173 oraz 193 (chodzi o budowanie zapytania) jest prawie identyczny. Kandydat na nową metodę?
Metody getContactReservationQuery, getContactDeveloperAgreementQuery, getContactFinalDeedQuery, getMeetingsQuery oraz getContactReservationAgreementQuery również są bardzo podobne. Ja bym napisał metodę, którą każda z tych metod wywołuje z odpowiednimi parametrami, bądź można zrobić z nich klasy (jedna klasę po czym te zapytania po niej dziedziczą).
Podobnie jest z getTotals i getTotalsForGroups.

Ogólnie, to czyta się to w OK. Jak zawsze zawsze coś można poprawić, ale jest spoko.

imo, chyba nie istnieje kod po tej stronie galaktyki, w którym zupełnie nie ma co poprawiać ;] - PR1V4T3_R 2016-02-17 21:13

Pozostało 580 znaków

2016-02-17 20:30
0

Nie mam pojęcia o co chodzi z tym Zzzzz, ktoś już się widać dobrał do tego kodu :D Dzięki za poświęcony czas i krytykę.

Pozostało 580 znaków

2016-02-28 22:03
0
  • Core_Model_Contact_SourceOfOriginReport CamelCase i Under_Score_Case w jednym?

  • 50-51 serio dwa foreach'e? Ja tu widzę nową metodę

  • @param null $data Zawsze chcesz żeby $data miała wartość null? Co to za jakieś czary, przecież metoda "getDividedIntoGroups" nawet nie korzysta z tego $data, oprócz tego żeby sprawdzić czy jest nullem.

  • Jak korzystasz z php 5.3+ to możesz spokojnie zamienić array() na [] żeby być bardziej pro.

  • getTotalsForGroups i getDividedIntoGroups robią prawie to samo (copy paste?). Ja bym wydzielił część wspólną do metody.

  • czemu

    $result = $this->_db->fetchAll($select);
    return $result;

    nie można zmienić na

    return $this->_db->fetchAll($select);
  • 163-164 po co ta niepotrzebna zmienna?

  • 163

    /**
    * @return object | BV_Db_Select
    */

    zamieniłbym na samo

    /**
    * @return BV_Db_Select
    */
  • getLeadQuery oraz getLeadDeletedQuery również mają duplikacje.

  • getContactQuery(),
    getContactReservationQuery(),
    getContactReservationAgreementQuery(),
    getContactDeveloperAgreementQuery(),
    getContactFinalDeedQuery(),
    getMeetingsQuery()
    też mają sporo duplikacji. Dodatkowo domyślam się że rzeczy które operują na Contact są bardziej podobne do siebie niż do całej reszty więc może nawet do innej klasy bym to wydzielił.

  • linia 346 @return BV_Db_Select|object na @return BV_Db_Select

  • $from[$column_name] = is_null($val) ? null : $val;

    a co to? nie można po prostu $from[$column_name] = $val

linia 376

array_filter($this->_columns_for_union, function($val) {
return is_null($val);
})

Jak nie podasz predykata (funkcji) jako drugi parametr do array_filter domyślnie usunie wszystkie falsifiable wpisy (nulle, false'y, 0, puste stringi, etc)
dopiero teraz zauważyłem że Ty chcesz same nulle zostawić, ten punkt wykreślam

</li> </ul>

Hints:

Teoretycznie, jeżeli ten kod nie polega na jakichś dziwnych kruczkach specyfiki języka, to można by zamienić
if (!empty($this->_search['created_from']))

na if ($this->_search['created_from'])

</li> </ul>
edytowany 1x, ostatnio: TomRiddle, 2016-02-28 22:08

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0