Witaj Gościu! ( Zaloguj | Rejestruj )

Forum PHP.pl

 
Reply to this topicStart new topic
> [Kohana]Kontrola jakości kodu
MateuszS
post 26.02.2013, 00:45:03
Post #1





Grupa: Zarejestrowani
Postów: 1 429
Pomógł: 195
Dołączył: 6.10.2008
Skąd: Kraków/Tomaszów Lubelski

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


Witam. Skończyłem mały projekt w Kohanie (prosta strona z newsami, galerią itp.). Nie ma sensu pokazywać strony bo najważniejsza jest w tym właśnie Kohana i szlifowanie jej. Dopiero zacząłem się go uczyć i chciałbym przejść swego rodzaju kontrolę jakości. Wiadomo, prawdopodobnie "forumowy sanepid" zamknąłby moją fabrykę kodu ale głównie chodzi mi o uzyskanie rad, co robię źle i jak to poprawić żeby było dobrze (albo chociaż lepiej). Przymierzam się w niedalekiej przyszłości do stworzenia większego projektu i chciałbym żeby to było zrobione PRO.

Chciałbym abyście przyjrzeli się kontrolerowi i modelowi newsów (dla panelu administracyjnego). Proszę nie traktować tego jako "kodu do oceny". Ja wiem że to do oceny się jeszcze nie nadaje, z tym że ja już nie bardzo wiem co mógłbym poprawić. Stąd moja prośba o "kontrolę" kodu. Wybaczcie durnowate komentarze w kodzie, czasami się gubiłem.

Kontroler:
http://wklej.org/id/966403/

Model:
http://wklej.org/id/966404/

Pozdrawiam

Ten post edytował MateuszS 26.02.2013, 01:07:52


--------------------
O! Zimniok :P
Go to the top of the page
+Quote Post
Damonsson
post 26.02.2013, 01:16:27
Post #2





Grupa: Zarejestrowani
Postów: 2 355
Pomógł: 533
Dołączył: 15.01.2010
Skąd: Bydgoszcz

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


Ciągłe odwoływanie się do funkcji statycznych, jest cechą tego frameworka? Czy Ty tak piszesz po prostu?

Ten post edytował Damonsson 26.02.2013, 08:40:59
Go to the top of the page
+Quote Post
toffiak
post 26.02.2013, 08:08:35
Post #3





Grupa: Zarejestrowani
Postów: 395
Pomógł: 80
Dołączył: 24.08.2009

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


Parę uwag z którymi nie wszyscy się zgodzą:
Kod jest rozwlekły, to znaczy twój podany przez Ciebie kod nie potrzebuje tylu komentarzy, tak wiem że często wymagane jest na uczelaniach aby komentować wszystko co się da ale komentarze odwracają uwagę od kodu właściwego. Tak naprawdę gdyby usunąć je wszystkie to i tak kod jest czytelny. Jedynie warto zachować komentarze nad metodami.
W przypadku "if-ów" i "else-ów" zawierających po jednej instrukcji powinieneś skorzystać z operatora trójargumentowego.
Kod kontrolera nie powinien przekraczać 30 linijek, choć oczywiście nie jest to reguła a raczej zalecenie.
Uwagi dotyczą kodu przedstawionego, w przypadku rozwiniętego modelu kod musi mieć więcej komentarzy, ponieważ są to sprawy indywidualne a nie zwykły CRUD.



--------------------
Go to the top of the page
+Quote Post
skowron-line
post 26.02.2013, 08:10:11
Post #4





Grupa: Zarejestrowani
Postów: 4 340
Pomógł: 542
Dołączył: 15.01.2006
Skąd: Olsztyn/Warszawa

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


Troche za dużo kodu w kontrolerze
  1. public function action_index()
  2. {
  3. //Pobieranie wszystkich newsów
  4. $_news = Model::factory('News')->news_get_all();
  5.  
  6. if (count($_news) > 0)
  7. {
  8. //Tabela newsów
  9. $content = View::factory("admin/news/news_table")->set('array_news', $_news);
  10. }
  11. else
  12. {
  13. //Błąd w przypadku braku newsów
  14. $content = $this->message("no_news_found");
  15. }
  16. //Ustawienie widoku
  17. $this->template->content = View::factory("admin/news/news")
  18. ->set('content', $content);
  19. }
  20.  

Validacja i dodawanie rekorów to w modelu się odbywa.
Moje metody odpowiedzialne za dodawanie i edycje rekordu wyglądają tak
  1. public function action_add()
  2. {
  3. $this->template->content = $this->form();
  4. }
  5.  
  6. public function action_edit()
  7. {
  8. $this->template->content = $this->form($this->model->get()->current());
  9. }
  10.  
  11. private function form($values = array())
  12. {
  13. // tu sprawdzam czy został wysłany post
  14. // i co zwraca metoda save w modelu
  15. return View::factory('form')->set('values', $values);
  16. }

Dodatkowo przenieś sobie metode message do kontrollera po którym dziedziczysz żebyś w każdym nie musiał sobie czegoś takiego dopisywać.
A model jak model nic szczególnego, tylko dlaczego wszystko na bool rzutujesz nie jest Ci potrzebne ID rekordu ktory dodałeś ? Liczba usuniętych / zmienionych rekordów
  1. /**
  2. * Odnalezienie newsa o podanym ID
  3. */
  4. return (DB::select()
  5. ->from("parys_news")
  6. ->where("ID", "=", $id)
  7. ->execute()
  8. ->count() > 0);


--------------------
I'm so fast that last night I turned off the light switch in my hotel room and was in bed before the room was dark - Muhammad Ali.
Peg jeżeli chcesz uprawiać sex to dzieci muszą wyjść, a jeżeli chcesz żeby był dobry ty też musisz wyjść - Al Bundy.

QueryBuilder, Mootools.net, bbcradio1::MistaJam
http://www.phpbench.com/
Go to the top of the page
+Quote Post
lukaskolista
post 26.02.2013, 13:25:05
Post #5





Grupa: Zarejestrowani
Postów: 872
Pomógł: 94
Dołączył: 31.03.2010

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


@Damonsson:
gdzie wedlug Ciebie ciagle odwoluje sie do metod statycznych poza implementacja wzorca fabryki(Class::factory()), ktory jakby nie bylo polega na tworzeniu metod statycznych zwracajacych instancje danej klasy?

Ogolnie jest dobrze, gdyby kazdy pisal przynajmniej na takim poziomie swiat bylby lepszy smile.gif

Pare uwag odemnie:
1. Model_News:
news_get_all() (i inne metody pobierajace dane) - po co zwracasz tablice (as_array()), IMO zwracanie iteratora jest lepszym rozwiazaniem m. in. z uwagi na dostep do roznych metod tego iteratora i mozliwosc ich rozszerzania.
news_exists($id) - gdybys uzywal wyzej wspomnianych iteratorow moglbys zrobic tak:
  1. $news = DB::select()
  2. ->from("parys_news")
  3. ->where("ID", "=", $id)
  4. ->execute()
  5. ->current;
  6.  
  7. return isset($news);


2. Obiekt walidacji moglbys zwracac w modelu, zamiast tworzyc go w kontrolerze.
3. Nie uzywasz transakcji na bazie danych sad.gif
4. Moze warto w Model_News zaimplementowac wzorzec fabryki (Model_News::factory())?

Korzystasz z przefiltrowanych danych (pobierasz je z obiektu walidatora), co sie bardzo chwali.
Go to the top of the page
+Quote Post
sabat24
post 26.02.2013, 14:01:45
Post #6





Grupa: Zarejestrowani
Postów: 175
Pomógł: 26
Dołączył: 13.09.2007
Skąd: Gdańsk

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


Z czysto konwencyjnych rzeczy, to z tego co pamiętam, Kohana nie lubi takich konstrukcji:
  1. if (warunek)
  2. instrukcja
  3. else
  4. instrukcja


poza użyciem continue i return zaleca się zawsze stosowanie klamer { } -> http://kohanaframework.org/3.3/guide/kohana/conventions

Jeśli używasz kohany 3.3, to nigdy nie używaj gołego $_POST, tak jak tu:
  1. $_post = Validation::factory($_POST)


W przypadku takiej konstrukcji:
  1. $view = View::factory("admin/news/message")->set("message", null);

Nie używaj set, tylko bind. Jeśli do zmiennej zostanie coś wpisanego później w kodzie 'na dole', to zostanie to 'podłączone' dzięki bind 'na górze'.

Ten post edytował sabat24 26.02.2013, 14:10:03
Go to the top of the page
+Quote Post
r4xz
post 26.02.2013, 15:21:54
Post #7





Grupa: Zarejestrowani
Postów: 673
Pomógł: 106
Dołączył: 31.12.2008

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


nie piszę w kohanie, ale z mojej strony powiem że preferuję zapis (zmienne, metody itp. cuda - zmyślone):
  1. <?php
  2.  
  3. class ...
  4.  
  5. public function foo() {
  6.  
  7. // 1 warunek
  8. $id = ...
  9. if( !$id )
  10.  
  11. return false;
  12.  
  13. // 2 warunek
  14. if( !$this -> request -> is('post'))
  15.  
  16. return false;
  17.  
  18. // i dopiero kod
  19. return View::....
  20.  
  21. }

zamiast:
  1. <?php
  2.  
  3. class ...
  4.  
  5. public function foo() {
  6.  
  7. // 1 warunek
  8. $id = ...
  9. if( $id ) {
  10.  
  11. // 2 warunek
  12. if( $this -> request -> is('post')) {
  13.  
  14. // i dopiero kod
  15. return View::....
  16.  
  17. }
  18. else
  19.  
  20. return false;
  21.  
  22. }
  23. else
  24.  
  25. return false;
  26.  
  27. }

musisz mieć tylko wtedy pewność np. jeśli zamiast return false masz przekierowanie/widok etc. że ta czynność blokuje wykonywanie dalszego kodu

PS pewnie ma tylu zwolenników, co i przeciwników? smile.gif


--------------------
Go to the top of the page
+Quote Post
skowron-line
post 26.02.2013, 15:32:39
Post #8





Grupa: Zarejestrowani
Postów: 4 340
Pomógł: 542
Dołączył: 15.01.2006
Skąd: Olsztyn/Warszawa

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


@r4xz kohana jaki i inne FW narzucają pewną stylistyke kodu, więc niech autor trzyma się tego co napisano
http://kohanaframework.org/3.3/guide/kohana/conventions


--------------------
I'm so fast that last night I turned off the light switch in my hotel room and was in bed before the room was dark - Muhammad Ali.
Peg jeżeli chcesz uprawiać sex to dzieci muszą wyjść, a jeżeli chcesz żeby był dobry ty też musisz wyjść - Al Bundy.

QueryBuilder, Mootools.net, bbcradio1::MistaJam
http://www.phpbench.com/
Go to the top of the page
+Quote Post
MateuszS
post 26.02.2013, 16:02:43
Post #9





Grupa: Zarejestrowani
Postów: 1 429
Pomógł: 195
Dołączył: 6.10.2008
Skąd: Kraków/Tomaszów Lubelski

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


Dokładnie. Na szczęście konwencji się jest łatwo nauczyć i faktycznie za późno zacząłem się tym interesować i mój kod był daleki od standardów Kohany.

Zainteresowało mnie to co napisał @skowron-line jeśli chodzi o ilość kodu w kontrolerze. Mianowicie spotkałem się (m. in. na forum polskim Kohany) ze stwierdzeniem że w modelu mamy tylko współpracę z bazą/obróbkę danych a walidacja odbywa się w kontrolerze. Mógłbyś pokazać jak u Ciebie wyglądałby model jeśliby przenieść tam walidację? Bo ten model przecież musi zwrócić jakieś błędy do kontrolera który to dopiero przekazuje je do widoku zgodnie z



Tak więc wolałem sobie oszczędzić komplikacji z przekazywaniem błędów/komunikatów/wartości z modelu do kontrolera i zostawiłem same wartości. Ale jestem ciekaw jak Wy to rozwiązaliście?

Co do komentarzy, tak jak pisałem, wiem że za dużo itp. ale już nie chciało mi się ich kasować przed wysłaniem, będę miał to na uwadze aby nie było ich za dużo.

Cytat
3. Nie uzywasz transakcji na bazie danych


Chodzi o jakieś współbieżne aktualizacje pól w MySQL?

Ten post edytował MateuszS 26.02.2013, 16:09:59


--------------------
O! Zimniok :P
Go to the top of the page
+Quote Post
Spawnm
post 26.02.2013, 16:30:42
Post #10





Grupa: Moderatorzy
Postów: 4 069
Pomógł: 497
Dołączył: 11.05.2007
Skąd: Warszawa




Za brak klamerek powinno się łamać klawiatury.

@MateuszS - twój obrazek nie pokazuje mvc tylko mvp.
Co do pisania kontrolerów to zawsze były osoby piszące gruby kontroler, chudy model i odwrotnie. Zaraz pewnie powstanie flame na poziomie czy camel case jest lepsze.
Go to the top of the page
+Quote Post
skowron-line
post 26.02.2013, 16:49:23
Post #11





Grupa: Zarejestrowani
Postów: 4 340
Pomógł: 542
Dołączył: 15.01.2006
Skąd: Olsztyn/Warszawa

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


Chudy kontroller i gruby model - taka filozofie wyznaje smile.gif
W kontrolerze tworząc instancje klasy przekazuje do modelu dane z post i get
  1. public function before()
  2. {
  3. parent::before();
  4. $this->model = new Model_Area(
  5. 'post' => $this->request->post(),
  6. 'get' => $this->request->query()
  7. )
  8. );
  9. }

i w modelu w metodzie save
  1. public function save()
  2. {
  3. $valid = Validation::factory($this->post)
  4. ->rule('name', 'not_empty')
  5. ->rule('color', 'not_empty');
  6.  
  7. if($valid->check() == false)
  8. {
  9. $this->errors = $valid->errors('error');
  10. return false;
  11. }
  12. // dodawanie lub edycja w zależności od parametru
  13. }

a w kontrolerze mam tak
  1. private function form($values = array())
  2. {
  3. if($this->request->post() !== array())
  4. {
  5. if($this->model->save() == false)
  6. {
  7. $error = $this->model->error();
  8. $values = $this->model->post();
  9. }
  10. else
  11. {
  12. //przekierowanie
  13. }
  14. }
  15.  
  16. return View::factory('area/form')
  17. ->set('values', $values);
  18. }


--------------------
I'm so fast that last night I turned off the light switch in my hotel room and was in bed before the room was dark - Muhammad Ali.
Peg jeżeli chcesz uprawiać sex to dzieci muszą wyjść, a jeżeli chcesz żeby był dobry ty też musisz wyjść - Al Bundy.

QueryBuilder, Mootools.net, bbcradio1::MistaJam
http://www.phpbench.com/
Go to the top of the page
+Quote Post
MateuszS
post 26.02.2013, 23:25:22
Post #12





Grupa: Zarejestrowani
Postów: 1 429
Pomógł: 195
Dołączył: 6.10.2008
Skąd: Kraków/Tomaszów Lubelski

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


  1. $this->errors = $valid->errors('error');


  1. $error = $this->model->error();


Po prostu przypisujesz tablicę do tablicy a potem metodą error() ją zwracasz tak?

@Spawnm, szybko znaleziony na Google więc możliwe. Ale układ jest ten sam co w MVC. Kontroler ma "dostęp" do widoku, model już nie.


PS. Czyli to czy umieszczę w kontrolerze walidację czy w modelu to kwestia obranej drogi tak? Nie jest błędem pisanie "grubego kontrolera"?

Ten post edytował MateuszS 26.02.2013, 23:26:42


--------------------
O! Zimniok :P
Go to the top of the page
+Quote Post
nmts
post 27.02.2013, 01:39:46
Post #13





Grupa: Zarejestrowani
Postów: 283
Pomógł: 34
Dołączył: 21.03.2008

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


Wszystko trzeba robić z umiarem, również za słuszniejsze uważam umieszczanie walidacji w modelu.

Co do kodu, przede wszystkim zauważ, że wykonujesz te same lub podobne czynności dodając i edytując - większość mogłaby być napisana tylko raz. Prawdopodobnie nie potrzebujesz też osobnych widoków - przeważnie formularz edycji i dodawania wygląda tak samo.
W action_edit i action_add zostawiłbym część odpowiedzialną za wyświetlanie widoku, natomiast zapis mógłby spocząć w odosobnieniu. Czyli dwie akcje, jeden widok, jeden zapis.


--------------------
Free Web Tools - narzędzia dla programistów, webdeveloperów i specjalistów seo...
Go to the top of the page
+Quote Post
skowron-line
post 27.02.2013, 08:05:59
Post #14





Grupa: Zarejestrowani
Postów: 4 340
Pomógł: 542
Dołączył: 15.01.2006
Skąd: Olsztyn/Warszawa

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


Cytat(MateuszS @ 26.02.2013, 23:25:22 ) *
Po prostu przypisujesz tablicę do tablicy a potem metodą error() ją zwracasz tak?

Tak
Cytat(MateuszS @ 26.02.2013, 23:25:22 ) *
PS. Czyli to czy umieszczę w kontrolerze walidację czy w modelu to kwestia obranej drogi tak? Nie jest błędem pisanie "grubego kontrolera"?

I tak i nie. Wiele osób napisze Ci ze za przetwarzanie danych odpowiedzialny jest model, a kontroler łączy model z widokiem.
  1. public function action_index()
  2. {
  3. $this->template->content = View::factory('area/index') //widok
  4. ->set('items', $this->model->get()); // model
  5. }

i to koniec, jeżeli chesz spawdzić i wyświetlić błąd związany z brakiem danych do wyświetlenia to możesz to zrobić w widoku, ale też w mojej opini model też by był dobrym miejscem na sprawdzenie, ale wszystko zależy od tego jak masz zaprojektowaną aplikację.


--------------------
I'm so fast that last night I turned off the light switch in my hotel room and was in bed before the room was dark - Muhammad Ali.
Peg jeżeli chcesz uprawiać sex to dzieci muszą wyjść, a jeżeli chcesz żeby był dobry ty też musisz wyjść - Al Bundy.

QueryBuilder, Mootools.net, bbcradio1::MistaJam
http://www.phpbench.com/
Go to the top of the page
+Quote Post
MateuszS
post 27.02.2013, 17:52:14
Post #15





Grupa: Zarejestrowani
Postów: 1 429
Pomógł: 195
Dołączył: 6.10.2008
Skąd: Kraków/Tomaszów Lubelski

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


Dzięki wszystkim za rady, postaram się ten kod przerobić tak jak zaleciliście (przynajmniej ten od newsów bo całego już nie dam rady, będę wiedział na przyszłość jak pisać)


--------------------
O! Zimniok :P
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: 28.03.2024 - 20:19