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

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

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.

Troche za dużo kodu w kontrolerze

[PHP] pobierz, plaintext 
public function action_index() 
{		
	//Pobieranie wszystkich newsów
	$_news = Model::factory('News')->news_get_all();
 
	if (count($_news) > 0)
	{	
		//Tabela newsów
        $content = View::factory("admin/news/news_table")->set('array_news', $_news);
	}
	else
	{
		//Błąd w przypadku braku newsów
        $content = $this->message("no_news_found");		
	}
	//Ustawienie widoku
	$this->template->content = 	View::factory("admin/news/news")
	->set('content', $content);
}
 
[PHP] pobierz, plaintext 

Validacja i dodawanie rekorów to w modelu się odbywa.
Moje metody odpowiedzialne za dodawanie i edycje rekordu wyglądają tak

[PHP] pobierz, plaintext 
public function action_add()
    {
        $this->template->content = $this->form();
    }
 
    public function action_edit()
    {
        $this->template->content = $this->form($this->model->get()->current());
    }
 
    private function form($values = array())
    {
        // tu sprawdzam czy został wysłany post
        // i co zwraca metoda save w modelu
        return View::factory('form')->set('values', $values);
     }
[PHP] pobierz, plaintext 

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

[PHP] pobierz, plaintext 
/**
	 * Odnalezienie newsa o podanym ID
	 */
return (DB::select()
		->from("parys_news")
		->where("ID", "=", $id)
		->execute()
		->count() > 0);
[PHP] pobierz, plaintext 

@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

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:

[PHP] pobierz, plaintext 
$news = DB::select()
	->from("parys_news")
	->where("ID", "=", $id)
	->execute()
	->current;
 
return isset($news);
[PHP] pobierz, plaintext 

2. Obiekt walidacji moglbys zwracac w modelu, zamiast tworzyc go w kontrolerze.
3. Nie uzywasz transakcji na bazie danych
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.

Z czysto konwencyjnych rzeczy, to z tego co pamiętam, Kohana nie lubi takich konstrukcji:

[PHP] pobierz, plaintext 
if (warunek)
instrukcja
else
instrukcja
[PHP] pobierz, plaintext 

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:

[PHP] pobierz, plaintext 
$_post = Validation::factory($_POST)
[PHP] pobierz, plaintext 

W przypadku takiej konstrukcji:

[PHP] pobierz, plaintext 
$view  = View::factory("admin/news/message")->set("message", null);
[PHP] pobierz, plaintext 

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

nie piszę w kohanie, ale z mojej strony powiem że preferuję zapis (zmienne, metody itp. cuda - zmyślone):

[PHP] pobierz, plaintext 
<?php
 
class ...
 
public function foo() {
 
	// 1 warunek
	$id = ...
	if( !$id )
 
		return false;
 
	// 2 warunek
	if( !$this -> request -> is('post'))
 
		return false;
 
	// i dopiero kod
	return View::....
 
}
[PHP] pobierz, plaintext 

zamiast:

[PHP] pobierz, plaintext 
<?php
 
class ...
 
public function foo() {
 
	// 1 warunek
	$id = ...
	if( $id ) {
 
		// 2 warunek
		if( $this -> request -> is('post')) {
 
			// i dopiero kod
			return View::....
 
		}
		else
 
			return false;
 
	}
	else
 
		return false;
 
}
[PHP] pobierz, plaintext 

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?

@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

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.



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

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.

Chudy kontroller i gruby model - taka filozofie wyznaje
W kontrolerze tworząc instancje klasy przekazuje do modelu dane z post i get

[PHP] pobierz, plaintext 
public function before()
    {
        parent::before();
        $this->model = new Model_Area(
                    array(
                        'post' => $this->request->post(),
                        'get'  => $this->request->query()
                    )
                );
    }
[PHP] pobierz, plaintext 

i w modelu w metodzie save

[PHP] pobierz, plaintext 
public function save()
    {
        $valid = Validation::factory($this->post)
                ->rule('name', 'not_empty')
                ->rule('color', 'not_empty');
 
        if($valid->check() == false)
        {
            $this->errors = $valid->errors('error');
            return false;
        }
        // dodawanie lub edycja w zależności od parametru
    }
[PHP] pobierz, plaintext 

a w kontrolerze mam tak

[PHP] pobierz, plaintext 
private function form($values = array())
    {
        if($this->request->post() !== array())
        {
            if($this->model->save() == false)
            {
                $error = $this->model->error();
                $values = $this->model->post();
            }
            else
            {
                //przekierowanie
            }
        }
 
        return View::factory('area/form')
                      ->set('values', $values);
    }
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
$this->errors = $valid->errors('error');
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
  $error = $this->model->error();
[PHP] pobierz, plaintext 

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

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.

Tak

I tak i nie. Wiele osób napisze Ci ze za przetwarzanie danych odpowiedzialny jest model, a kontroler łączy model z widokiem.

[PHP] pobierz, plaintext 
public function action_index()
    {
        $this->template->content = View::factory('area/index') //widok
                ->set('items', $this->model->get()); // model
    }
[PHP] pobierz, plaintext 

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

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