Witaj Gościu! ( Zaloguj | Rejestruj )

Forum PHP.pl

 
Reply to this topicStart new topic
> [PHP]Refaktoryzacja kontrolerów Symfony. Proszę o wskazówki.
porzeczki
post 28.09.2016, 02:10:47
Post #1





Grupa: Zarejestrowani
Postów: 144
Pomógł: 0
Dołączył: 15.09.2016
Skąd: Warszawa

Ostrzeżenie: (0%)
-----


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ć.

Ten post edytował porzeczki 28.09.2016, 02:19:12
Go to the top of the page
+Quote Post
com
post 29.09.2016, 18:35:26
Post #2





Grupa: Zarejestrowani
Postów: 3 032
Pomógł: 366
Dołączył: 24.05.2012

Ostrzeżenie: (0%)
-----


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?
Go to the top of the page
+Quote Post
porzeczki
post 1.10.2016, 11:03:13
Post #3





Grupa: Zarejestrowani
Postów: 144
Pomógł: 0
Dołączył: 15.09.2016
Skąd: Warszawa

Ostrzeżenie: (0%)
-----


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?
Go to the top of the page
+Quote Post
kpt_lucek
post 1.10.2016, 12:48:46
Post #4





Grupa: Zarejestrowani
Postów: 428
Pomógł: 77
Dołączył: 10.07.2011
Skąd: Warszawa

Ostrzeżenie: (0%)
-----


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:
  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. }


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


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.
  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 );

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.
  1. if(!$cart_obiekt->session->has('cart')) {
  2. throw new \Exception('Koszyk pusty.');
  3. }


---

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

  1. $session->set('cart',$cart );


---

5.
Używaj repozytoriów

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


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ć:
  1. /** @var EntityManager $em */
  2. $em = $this->get('doctrine.orm.entity_manager');
  3. $query = $em->getRepository('AppBundle:Ksiazka')
  4. ->createQueryBuilder('ksiazka')
  5. ->getQuery();


I wiele więcej smile.gif

Ten post edytował kpt_lucek 1.10.2016, 12:55:25


--------------------


Cytat
There is a Bundle for that
Lukas Kahwe Smith - October 31th, 2014
Go to the top of the page
+Quote Post
porzeczki
post 1.10.2016, 13:01:36
Post #5





Grupa: Zarejestrowani
Postów: 144
Pomógł: 0
Dołączył: 15.09.2016
Skąd: Warszawa

Ostrzeżenie: (0%)
-----


bardzo bardzo dziękuję
Go to the top of the page
+Quote Post
com
post 1.10.2016, 13:39:45
Post #6





Grupa: Zarejestrowani
Postów: 3 032
Pomógł: 366
Dołączył: 24.05.2012

Ostrzeżenie: (0%)
-----


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
Go to the top of the page
+Quote Post
porzeczki
post 9.10.2016, 23:26:00
Post #7





Grupa: Zarejestrowani
Postów: 144
Pomógł: 0
Dołączył: 15.09.2016
Skąd: Warszawa

Ostrzeżenie: (0%)
-----


a jak odchudzić poniższy kontroler?

  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.  

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.
Go to the top of the page
+Quote Post
kpt_lucek
post 10.10.2016, 01:35:41
Post #8





Grupa: Zarejestrowani
Postów: 428
Pomógł: 77
Dołączył: 10.07.2011
Skąd: Warszawa

Ostrzeżenie: (0%)
-----


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

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

1. Masz dostępny serwis session
2. Masz dostępną metodę
  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');


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

Cytat
  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);


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
  1. return $this->redirect($this->generateUrl('potwierdzenie'));
  2.  
  3. // a może po prostu tak: return $this->redirectToRoute('potwierdzenie');


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".



--------------------


Cytat
There is a Bundle for that
Lukas Kahwe Smith - October 31th, 2014
Go to the top of the page
+Quote Post
porzeczki
post 11.10.2016, 19:24:17
Post #9





Grupa: Zarejestrowani
Postów: 144
Pomógł: 0
Dołączył: 15.09.2016
Skąd: Warszawa

Ostrzeżenie: (0%)
-----


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.

Ten post edytował porzeczki 11.10.2016, 19:25:54
Go to the top of the page
+Quote Post
kpt_lucek
post 11.10.2016, 22:19:21
Post #10





Grupa: Zarejestrowani
Postów: 428
Pomógł: 77
Dołączył: 10.07.2011
Skąd: Warszawa

Ostrzeżenie: (0%)
-----


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".

Ten post edytował kpt_lucek 11.10.2016, 22:33:18


--------------------


Cytat
There is a Bundle for that
Lukas Kahwe Smith - October 31th, 2014
Go to the top of the page
+Quote Post
com
post 11.10.2016, 22:39:09
Post #11





Grupa: Zarejestrowani
Postów: 3 032
Pomógł: 366
Dołączył: 24.05.2012

Ostrzeżenie: (0%)
-----


porzeczki pytanie tylko poco to robisz? i czemu tak
Go to the top of the page
+Quote Post
porzeczki
post 12.10.2016, 13:33:23
Post #12





Grupa: Zarejestrowani
Postów: 144
Pomógł: 0
Dołączył: 15.09.2016
Skąd: Warszawa

Ostrzeżenie: (0%)
-----


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
  1. ZamowienieProdukt(new Ksiazka)
) Muszę wrócić do podstaw Doctrine.
Go to the top of the page
+Quote Post

Reply to this topicStart new topic
1 Użytkowników czyta ten temat (1 Gości i 0 Anonimowych użytkowników)
0 Zarejestrowanych:

 



RSS Wersja Lo-Fi Aktualny czas: 18.04.2024 - 15:46