Pomoc - Szukaj - Użytkownicy - Kalendarz
Pełna wersja: [PHP]Refaktoryzacja kontrolerów Symfony. Proszę o wskazówki.
Forum PHP.pl > Forum > Przedszkole
porzeczki
28.09.2016, 02:10:47
Chciałbym zabrać się za refaktoryzację projektu (pierwszego i jedynego jaki zrobiłem - praca dyplomowa inż). Robiłem go nie zwracając uwagi na zasady porządnego kodu, bo ich nie znałem, projekt miał wyglądać dobrze w przeglądarce internetowej a nie w IDE. Teraz, gdy naczytałem się stosu książek i best practices głowa pęka i nie potrafię tego natłoku nowych informacji posortować w głowie i wykorzystać.

Jakby komuś się strasznie nudziło to byłbym bardzo wdzięczny za wskazanie głupot w tych kontrolerach i ogólnych wskazówek typu: "takie rzeczy to przenieś i używaj jako usług", " z tego najlepiej zrobić listener", "takie coś nie powinno być w kontrolerze".

Będę wdzięczny za każdą, nawet najbardziej ogólną, nie związaną z tą aplikacją uwagę.

github.com/.../Bundle/Controller/


edit:
chociaż tak sobie wszedłem teraz w pierwszy lepszy kontroler to zacząłem wątpić by komuś chciało się rozkminiać o co tam w ogóle chodzi,a bez zrozumienia o co chodzi chyba nie da się strzelać poradami jak to refaktoryzować.
com
29.09.2016, 18:35:26
Pierwsze co bym zrobił to posprzątał ten kod, bo to na pewno nie jest wersja produkcyjna. Wywalił zbędne komentarze i kod w nich. Owszem bez znajomości tego wszystkiego nie dostaniesz gotowych rad, ale ważne, żeby trzymać się zasad SOLID, DRY, KISS itp. Generalnie widzę masz metody w których robisz "magie" na kilkanaście ekranów, wiec to bym na pewno podał refaktoringowi. Tam gdzie się da o else zapomnij, zwiększysz tym sposobem w prosty sposób czytelność metod. Nazwy kontrolerów po polsku? Komentarze polsko - angielskie?
porzeczki
1.10.2016, 11:03:13
dzięki

Cytat(com @ 29.09.2016, 19:35:26 ) *
masz metody w których robisz "magie" na kilkanaście ekranów.

co to znaczy, że robię magię? jaki to jest magiczny kod?
Cytat(com @ 29.09.2016, 19:35:26 ) *
Tam gdzie się da o else zapomnij, zwiększysz tym sposobem w prosty sposób czytelność metod.

masz na myśli to, by przenieść taki kod z kontrolera czy żeby zastąpić else kolejnym ifem lub switchem?
kpt_lucek
1.10.2016, 12:48:46
Cytat(porzeczki @ 1.10.2016, 12:03:13 ) *
[...]
masz na myśli to, by przenieść taki kod z kontrolera czy żeby zastąpić else kolejnym ifem lub switchem?

1.
Nie

Dla przykładu:
[PHP] pobierz, plaintext 
  1. if($session->has('cart'))//jeśli zmienna sesji cart juz jest to:
  2. {         
  3.     $cart = $session->get('cart');
  4.     $cartquantity = array_sum($cart)  ;        
  5. }else{
  6.     $cartquantity = 0;
  7. }
[PHP] pobierz, plaintext


Możesz zamienić w ten sposób:
[PHP] pobierz, plaintext 
  1. $cartquantity = 0;
  2.  
  3. if($session->has('cart'))
  4. {         
  5.     $cart = $session->get('cart');
  6.     $cartquantity = array_sum($cart);        
  7. }
[PHP] pobierz, plaintext


Wartość logiczna jest ta sama, a kodu mniej.

---

2.
Używaj serwisów, wrzucaj logikę tam, im niżej w hierarchii tym lepiej, bo mniejsza duplikacja kodu.
[PHP] pobierz, plaintext 
  1.         $session = $request->getSession();
  2.  
  3.         if($session->has('cart'))//jeśli zmienna sesji cart juz jest to:
  4.         { 
  5.             $cart = $session->get('cart');
  6.             if(!array_key_exists($isbn,$cart))//jeśli isbn nie jest w koszu
  7.             {
  8.                 $cart[$isbn]=1; // = 1
  9.             }
  10.             else
  11.             {
  12.               $cart[$isbn]++; //to zwiększ wartość ++
  13.             }
  14.         }
  15.         else
  16.         {
  17.             $cart[$isbn]=1;
  18.         }
  19.             $session->set('cart',$cart );
[PHP] pobierz, plaintext

Pomyśl ile razy robisz logikę podobną do powyższej, nie prościej zarejestrować serwis który jako dependency ma serwis @session i bezpośrednio na nim bazuje?

---

3.
Twórz własny Exception, tak aby opisywał zdarzenie, bo inaczej na dłuższą metę się nie połapiesz.
[PHP] pobierz, plaintext 
  1. if(!$cart_obiekt->session->has('cart')) {
  2.     throw new \Exception('Koszyk pusty.');
  3. }
[PHP] pobierz, plaintext


---

4.
Używaj CONSTów, zwłaszcza jak współdzielisz nazwy parametrów pomiędzy uslugami.

[PHP] pobierz, plaintext 
  1. $session->set('cart',$cart );
[PHP] pobierz, plaintext


---

5.
Używaj repozytoriów

[PHP] pobierz, plaintext 
  1. $em    = $this->get('doctrine.orm.entity_manager');
  2. $dql   = "SELECT a FROM AppBundle:Ksiazka a";
  3. $query = $em->createQuery($dql);
[PHP] pobierz, plaintext


Powyższy kod możesz wstawić w Repozytorium i korzystać z niego w wielu miejscach bez potrzeby duplikowania go (nawiązuje do #2).

Poza tym, to co masz wyżej i tak możesz uprościć:
[PHP] pobierz, plaintext 
  1. /** @var EntityManager $em */
  2. $em    = $this->get('doctrine.orm.entity_manager');
  3. $query = $em->getRepository('AppBundle:Ksiazka')
  4. 	            ->createQueryBuilder('ksiazka')
  5. 	            ->getQuery();
[PHP] pobierz, plaintext


I wiele więcej smile.gif
porzeczki
1.10.2016, 13:01:36
bardzo bardzo dziękuję
com
1.10.2016, 13:39:45
Cytat
co to znaczy, że robię magię? jaki to jest magiczny kod?

to znaczy, że metody robią zbyt wiele i kod jest za bardzo złożony i zbyt długi. (Reguła KISS),
a na drugie już kolega odpowiedział wink.gif
porzeczki
9.10.2016, 23:26:00
a jak odchudzić poniższy kontroler? 

[PHP] pobierz, plaintext 
  1.     /**
  2.      * Formularz danych adresowych klienta.
  3.      * 
  4.      * @Route("/zamawiam", name="zamawiam")
  5.      */
  6.     public function zamawiamAction(Request $request)
  7.     {
  8.         $session = $request->getSession();
  9.         $em = $this->getDoctrine()->getManager();
  10.         $logged = $this->get('security.authorization_checker')
  11.                 ->isGranted('IS_AUTHENTICATED_FULLY');
  12.  
  13.         if(!$session->has('cart')){
  14.             throw new \Exception('Koszyk pusty.');
  15.         }
  16.  
  17.  
  18.         $idlogowanie = $this->getUser()->getId();
  19.         $klient= $this->getDoctrine()
  20.             ->getRepository('AppBundle:Klient')
  21.             ->findOneBy(['idlogowanie' => $idlogowanie]);
  22.         // jeśli zalogowany nigdy nie wypełniał formularza dostawy lub jeśli niezalogowany
  23.         if (!$klient){$klient = new Klient();}
  24.  
  25.  
  26.         $form= $this->createForm(DostawaType::class, $klient, array(
  27.         'attr' => array('class' => 'form_dostawa')));
  28.  
  29.         $form->handleRequest($request);
  30.  
  31.         if ($form->isValid())
  32.         {
  33.             //linijka dla zalogowanego użytkownika
  34.             if ($logged){$klient->setIdlogowanie($this->getUser());}
  35.             //wypełnienie tabeli Zamowienie
  36.             $zamowienie = new Zamowienie();
  37.             $zamowienie->setIdklient($klient);
  38.             $status = $this->getDoctrine()
  39.                     ->getRepository('AppBundle:Status')
  40.                     ->find('1');
  41.             $zamowienie->setIdstatus($status);
  42.             $zamowienie->setDatazlozeniacurrent();
  43.             //wypełnienie tabeli Zamowienie_Produkt
  44.             $cart = $session->get('cart');
  45.             foreach ($cart as $isbn => $quantity)
  46.             {   
  47.                 $ksiazka = $this->getDoctrine()
  48.                     ->getRepository('AppBundle:Ksiazka')
  49.                     ->find($isbn);
  50.                 $isbn=$ksiazka->getIsbn();        
  51.                 $tytul=$ksiazka->getTytul();        
  52.                 $autor=$ksiazka->getAutor();
  53.                 $cena=$ksiazka->getCena();        
  54.                 $rokwydania=$ksiazka->getRokwydania();        
  55.                 $ilosc=$quantity;        
  56.                 $zamowienieProdukt = new ZamowienieProdukt();
  57.                 $zamowienieProdukt->setIdzamowienie($zamowienie); 
  58.                 $zamowienieProdukt->setIsbn($ksiazka);
  59.                 $zamowienieProdukt->setTytul($tytul);
  60.                 $zamowienieProdukt->setAutor($autor);
  61.                 $zamowienieProdukt->setCenaproduktu($cena);
  62.                 $zamowienieProdukt->setRokwydania($rokwydania);
  63.                 $zamowienieProdukt->setIlosc($ilosc);
  64.                 $em->persist($zamowienieProdukt);
  65.             }  
  66.  
  67.             $em->persist($klient);
  68.             $em->persist($zamowienie);
  69.             $em->flush();    
  70.  
  71.             $idzamowienia=$zamowienie->getIdzamowienie();
  72.             $request->getSession()->getFlashBag()->add(
  73.                 'idzamowienie',
  74.                 $idzamowienia);
  75.             return $this->redirect($this->generateUrl('potwierdzenie')
  76.                     );
  77.         }
  78.  
  79.  
  80.         return $this->render('AppBundle:Cart:zamawiam.html.twig',[
  81.             'form' => $form->createView()
  82.         ]);
  83.  
  84.     }
  85.  
[PHP] pobierz, plaintext

czy listenerów i usług ładnie jest używać do kodu jednokrotnego użytku byle odchudzić kontroler? Np gdybym chciał wepchnąć cały kod z powyższego if($form->isValid()){ do listenera/usługi.
kpt_lucek
10.10.2016, 01:35:41
Cytat
czy listenerów i usług ładnie jest używać do kodu jednokrotnego użytku byle odchudzić kontroler? Np gdybym chciał wepchnąć cały kod z powyższego if($form->isValid()){ do listenera/usługi.


Tak, stwierdzenie "odchudzenia" kontrolera nie do końca jest tutaj poprawne, zgodnie z założeniami SOLID, a dokładniej samego SingleResponsibilityPrinciple, powinieneś budować architekturę tak, żeby każdy z wytworzonych przez Ciebie obiektów miał 1 odpowiedzialność, nie zawsze niestety jest to takie proste, polecam przeczytać TO.

W SF2/3 założeniem kontrolera (mówiąc dokładniej, akcji w kontrolerze) jest zebranie wszystkich odpowiednich danych, przemielenie tego poprzez usługi (serwisy) i wyplucie Response'a, tak powinna wyglądać "logika" akcji w kontrolerze.

Jak możesz uprościć kod który podałeś.
Moja prywatna opinia

[PHP] pobierz, plaintext 
  1. $session = $request->getSession();
  2.         $em = $this->getDoctrine()->getManager();
  3.         $logged = $this->get('security.authorization_checker')
  4.                 ->isGranted('IS_AUTHENTICATED_FULLY');
[PHP] pobierz, plaintext

1. Masz dostępny serwis session
2. Masz dostępną metodę
[PHP] pobierz, plaintext 
  1. $logged = $this-getUser() instanceof UserInterface;
  2. // lub po prostu
  3. $this->isGranted('ROLE_USER');
  4.  
  5. //Jak chcesz wywalić 403
  6. $this->denyAccessUnlessGranted('ROLE_USER');
[PHP] pobierz, plaintext


---Ogólnie polecam sprawdzenie metod obiektów które rozszerzasz smile.gif

Cytat
[PHP] pobierz, plaintext 
  1.              $ksiazka = $this->getDoctrine()
  2.                     ->getRepository('AppBundle:Ksiazka')
  3.                     ->find($isbn);
  4.                 $isbn=$ksiazka->getIsbn();        
  5.                 $tytul=$ksiazka->getTytul();        
  6.                 $autor=$ksiazka->getAutor();
  7.                 $cena=$ksiazka->getCena();        
  8.                 $rokwydania=$ksiazka->getRokwydania();        
  9.                 $ilosc=$quantity;        
  10.                 $zamowienieProdukt = new ZamowienieProdukt();
  11.                 $zamowienieProdukt->setIdzamowienie($zamowienie); 
  12.                 $zamowienieProdukt->setIsbn($ksiazka);
  13.                 $zamowienieProdukt->setTytul($tytul);
  14.                 $zamowienieProdukt->setAutor($autor);
  15.                 $zamowienieProdukt->setCenaproduktu($cena);
  16.                 $zamowienieProdukt->setRokwydania($rokwydania);
  17.                 $zamowienieProdukt->setIlosc($ilosc);
[PHP] pobierz, plaintext


Machnij do tego jakiś transformer o ile już musisz robić to w taki sposób, lub przemyśl, czy Twój formularz na prawdę musi działać tak topornie? Jeżeli musi, to może To Ci jakoś ułatwi pracę.

Cytat
[PHP] pobierz, plaintext 
  1. return $this->redirect($this->generateUrl('potwierdzenie'));
  2.  
  3. // a może po prostu tak: return $this->redirectToRoute('potwierdzenie');
[PHP] pobierz, plaintext


Ogólnie, to do ogrania forma użyam handlera, a ten z innych udogodnień, które przejmują pewną część procesu.

Możesz zainteresować się takim rozwiązaniem, napisać własne, lub zupełnie to olać, ale pamiętaj, że im bardziej coś podzielisz, tym łatwiej będzie Ci wpływać na kod w przyszłości, testować go jak i wymieniać poszczególne "komponenty".

porzeczki
11.10.2016, 19:24:17
dziękuję. Jedna rzecz
Cytat(kpt_lucek @ 10.10.2016, 02:35:41 ) *
Machnij do tego jakiś transformer

może źle odczytałeś sens tego fragmentu kodu. A może ja nie rozumiem możliwości Data Tranformer. W tym fragmencie ja nie modyfikuję formatu danych, może w głupi sposób(?), ale po prostu ustawiam pola jednego obiektu entity na podstawie pól drugiego.
kpt_lucek
11.10.2016, 22:19:21
Tym bardziej powinieneś mieć to oddelegowane i przeniesione, do tego masz transformery, fizycznie w SF masz (o ile dobrze pamiętam) DataTransformerInterface? On ma metode transform i reverseTransform.

Oczywiście w tym wypadku nikt nie każe Ci używać tego interface'u, po prostu dobrze jest to oddelegować gdzie indziej, chociażby po to, aby logika za to odpowiadająca była dostępna "dla innych" usług, lub też po prostu: "żeby było czyściej".
com
11.10.2016, 22:39:09
porzeczki pytanie tylko poco to robisz? i czemu tak
porzeczki
12.10.2016, 13:33:23
a jak?

(zamówienie składa się z wielu ZamowienieProdukt. W pętli tworzę obiekty ZamowienieProdukt a dane biorę z obiektu Ksiazka. No nie wiem, nie potrafię tego dziś obronić, dawno to robiłem, Doctrine od roku nie używałem, więc mądrzejszy dziś nie jestem. Pewnie chodzi ci o to że mógłbym tworząc ZamowienieProdukt od razu wcisnąć do niego obiekt Ksiazke 
[PHP] pobierz, plaintext 
  1. ZamowienieProdukt(new Ksiazka)
[PHP] pobierz, plaintext
) Muszę wrócić do podstaw Doctrine.
To jest wersja lo-fi głównej zawartości. Aby zobaczyć pełną wersję z większą zawartością, obrazkami i formatowaniem proszę kliknij tutaj.
Invision Power Board © 2001-2025 Invision Power Services, Inc.