[skrypt] Kohana ocena kodu Opcje

[bladeer]

Witam, Uczę się kohany, i proszę was o ocene kodu napisanego właśnie na kohanie. Wszelaka krytyka wskazana
Skrypt służy do logowania, rejestracj, zmiany hasła itp dla małej wyszukiwarki firm, praca robiona na praktyki szkolne.

Kontroler:

[PHP] pobierz, plaintext 
<?php
 
defined('SYSPATH') or die('No direct script access.');
 
class Controller_User extends Controller_Layout
{
 
    public function action_index()
    {
 
    }
 
    public function action_login()
    {
        // If user already signed-in
        if (Auth::instance()->logged_in() != 0) {
            //redirect to the user account
            HTTP::redirect('index/index');
        }
 
 
        $this->template->content = View::factory('user/login');
        if ($this->request->post()) {
            Helper::filter_post($this, array(
                'username',
                'password',
                'remember'
            ));
 
            $user = ORM::factory('User');
            $validation = $user->validate_login($this->request->post());
 
            if ($validation->check()) {
                $user = Auth::instance()->login(
                        $this->request->post('username'), $this->request->post('password'), $this->request->post('remember')
                );
                if ($user) {
                    HTTP::redirect('index/index');
                } else {
                    $this->template->errors = array('Niepoprawny login lub hasło');
                }
            } else {
                $this->template->errors = $validation->errors('register');
            }
        }
    }
 
    public function action_logout()
    {
        // Log user out
        Auth::instance()->logout();
        // Redirect to login page
        HTTP::redirect('user/login');
    }
 
    public function action_register()
    {
        // If user already signed-in
        if (Auth::instance()->logged_in() != 0) {
            //redirect to the user account
            HTTP::redirect('user/profil');
        }
 
 
        if ($this->request->post()) {
            Helper::filter_post($this, array(
                'username',
                'email',
                'username',
            ));
 
            $user = ORM::factory('User');
            $validation = $user->validate_register($this->request->post());
            if ($validation->check()) {
 
                $user->username = $this->request->post('username');
                $user->password = $this->request->post('password');
                $user->email = $this->request->post('email');
                $user->save();
 
                $login_role = new Model_Role(array('name' => 'login'));
                $user->add('roles', $login_role);
 
                Helper::notice('Rejestracja ukończona. Teraz możesz się zalogować');
            } else {
                $this->template->errors = $validation->errors('register');
            }
        }
 
        $this->template->content = View::factory('user/register');
    }
 
    public function action_profil()
    {
        Helper::chceck_login();
 
        $this->template->content = View::factory('user/profil');
    }
 
    public function action_changepass()
    {   
        $user = new Model_User;
        $validation = $user->validate_change_pass($this->request->post());
        if ($validation->check()) {
            //zmieniamy hasło
            $user = ORM::factory('User',  Auth::instance()->get_user()->id);
            $user->password = $this->request->post('password');
            $user->save();
 
            $user->saved() ? Helper::notice('Hasło zostało zmienione') : Helper::notice('Błąd danych. Hasło nie zostało zmienione') ;
        } else {
            //jeśli validacja się nie powiodła , uruchamiamy kotroler profilu. 
            //Wysyłamy do niego także informacje o błędach
             $this->template->errors = $validation->errors('change_pass');
             $this->action_profil();
        }
    }
 
    public function action_changeemail()
    {
        $user = new Model_User;
        $validation = $user->validate_change_email($this->request->post());
        if ($validation->check()) {
            //jeśli wszystko przebiegło ok, zmieniamy email
            $user = ORM::factory('User',  Auth::instance()->get_user()->id);
            $user->password = $this->request->post('email');
            $user->save();
 
            $user->saved() ? Helper::notice('Email został zmieniony') : Helper::notice('Błąd danych. Email nie został zmieniony') ;
 
        } else {
            $this->template->errors = $validation->errors('change_email');
            $this->action_profil();
        }
 
    }
 
}
[PHP] pobierz, plaintext 

Model: (wydaje mi się że nie napisałem go poprawnie , i w normalnej aplikacju było by to nie do przyjecia, ale nie mam pomysłu jak to "unormalnić")

[PHP] pobierz, plaintext 
<?php
 
class Model_User extends Model_Auth_User
{
 
    protected $rules_register = array(
        'username' => array(
            array('not_empty'),
            array('min_length', array(':value', 3)),
            array('max_length', array(':value', 48)),
            array('regex', array(':value', '/^[-\pL\pN_.]++$/uD')),
            array('Model_User::check_unique', array('username', ':value')),
        ),
        'email' => array(
            array('not_empty'),
            array('min_length', array(':value', 3)),
            array('max_length', array(':value', 127)),
            array('email'),
            array('Model_User::check_unique', array('email', ':value')),
        ),
        'email_confirm' => array(
            array('matches', array(':validation', ':field', 'email')),
        ),
        'password' => array(
            array('not_empty'),
            array('min_length', array(':value', 3)),
            array('max_length', array(':value', 48)),
        ),
        'password_confirm' => array(
            array('matches', array(':validation', ':field', 'password')),
        ),
    );
 
    protected $rules_login = array(
        'username' => array(
            array('not_empty'),
            array('min_length', array(':value', 3)),
            array('max_length', array(':value', 48)),
            array('regex', array(':value', '/^[-\pL\pN_.]++$/uD')),
        ),
        'password' => array(
            array('not_empty'),
            array('min_length', array(':value', 3)),
            array('max_length', array(':value', 48)),
        ),
        'email' => array(
            array('not_empty'),
            array('email'),
        )
    );
 
    public function validate_register($postvalues)
    {
        $array = Validation::factory($postvalues)
                ->rules('username', $this->rules_register['username'])
                ->rules('email', $this->rules_register['email'])
                ->rules('email_confirm', $this->rules_register['email_confirm'])
                ->rules('password', $this->rules_register['password'])
                ->rules('password_confirm', $this->rules_register['password_confirm'])
        ;
 
        return $array;
    }
 
    public function validate_login($postvalues)
    {
        $array = Validation::factory($postvalues)
                ->rules('username', $this->rules_login['username'])
                ->rules('password', $this->rules_login['password'])
        ;
 
        return $array;
    }
 
    /**
     * validacja zmiany hasła.
     * @param array $postvalues
     */
    public function validate_change_pass($postvalues)
    {
        $array = Validation::factory($postvalues)
                ->rules('password_old', $this->rules_register['password'])
                ->rule('password_old','Model_User::check_pass')
                ->rules('password', $this->rules_register['password'])
                ->rules('password_confirm', $this->rules_register['password_confirm'])
        ;
 
        return $array;
    }
 
    /**
     * validacja zmiany emaila
     * @param type $postvalues
     */
    public function validate_change_email($postvalues)
    {
        $array = Validation::factory($postvalues)
                ->rules('password_email', $this->rules_register['password'])
                ->rule('password_email','Model_User::check_pass')
                ->rules('email',$this->rules_login['email'])
                ->rule('email_confirm','matches',array(':validation', ':field', 'email'));
        ;
 
        return $array;
    }
 
    public static function check_unique($column, $value)
    {
        // Check if the username already exists in the database
        return !DB::select(array(DB::expr("COUNT($column)"), 'total'))
                        ->from('users')
                        ->where($column, '=', $value)
                        ->execute()
                        ->get('total');
    }
 
    /**
     * Sprawdza czy podane hasło jest zgodne z hasłem
     * aktualnie zalogowanego usera
     * @param string $pass Nie zhashowane hasło 
     */
    public static function check_pass($pass)
    {
        $pass_hash = Auth::instance()->hash_password($pass);
        return ( Auth::instance()->get_user()->password == $pass_hash) ? true : false ;
    }
 
}
 
[PHP] pobierz, plaintext 

Funkcja Helper::filter_post

[PHP] pobierz, plaintext 
/**
     * Filtruje liste zmiennych z post
     * @param object $controller
     * @param array $list
     */
    static public function filter_post($controller,$list)
    {
        foreach ($list as $value) {
            $controller->request->post($value, Helper::filter($controller->request->post($value)));
        }
    }
[PHP] pobierz, plaintext 

Ten post edytował bladeer 16.05.2013, 18:09:56

[Euzebio11]

Szczerze mówiać wyglada całkiem całkiem ok na pewno jest to twój pierwszy skrypt

[phpion]

Jest bardzo fajnie, ale można się przyczepić:
1. Może warto przypisać Auth::instance() do składowej kontrolera? Wówczas odwoływałbyś się do $this->auth zamiast każdorazowo wzywać instancję klasy.
2. Auth::logged_in() zwraca boola, więc możesz sprawdzać === TRUE zamiast != 0.
3. Po przejściu walidacji operujesz na danych z $this->request->post(). Sugerowałbym na $validation->as_array(). W KO3 nie jest to w sumie konieczne, ale w KO2 już tak (z racji filtrów). Może to moje przyzwyczajenie z KO2, ale wolę działać na danych, które faktycznie zostały sprawdzone (czyli z obiektu Validation, a nie Request).
4. Wszystkie komunikaty tekstowe proponuję przepuszczać przez __(). W przypadku konieczności tłumaczenia softu na inny język nie musisz dotykać kontrolerów.
5. W metodzie action_changeemail() masz błąd:

[PHP] pobierz, plaintext 
$user->password = $this->request->post('email'); // zmieniasz hasło czy email? :)
[PHP] pobierz, plaintext 

6. Po poprawnym wysłaniu formularza i zapisaniu danych powinieneś dokonać przekierowania użytkownika na ten sam adres żeby uniknąć ponownego przesyłania formularza przy odświeżeniu strony.
7. Twój model jest całkiem OK, podoba mi się wydzielenie osobnych metod walidacyjnych dla różnych przypadków, a nie kombinowanie z jedną metodą.
8. Metoda check_unique może być napisana wydajniej (zamiast COUNT() robisz SELECT 1 FROM... i ograniczasz na LIMIT 1 - jeśli w wyniku dostaniesz jakikolwiek rekord (1) to znaczy, że user istnieje).

Generalnie: fajnie

[bladeer]

Ok dzięki za opinie co do funkcji Helper::notice to ona poprostu przekierowywuje na strone index/notice i wyświetla templatke z podanym w parametrze textem. Dzięki za znaleziony błąd.
Nie jest to mój pierwszy skrypt, wcześniej pisałem pod kohaną ale szkoda pokazywać tego kodu. Walidacja tam praktycznie nie istniała.

PS: Ok to zobaczymy co jeszcze powie mój "szef" ;p

Ten post edytował bladeer 17.05.2013, 17:42:06

[wujek2009]

Nie mogę rozgryźć funkcji helpera - filter_post a dokładniej zapisu: Helper::filter - to jest jakaś nowa funkcja z K3.3? Filtrowanie ogólnie w modelu już można zrobić przykład z Auth (ORM):

[PHP] pobierz, plaintext 
	/**
	 * Filters to run when data is set in this model. The password filter
	 * automatically hashes the password when it's set in the model.
	 *
	 * @return array Filters
	 */
	public function filters()
	{
		return array(
			'password' => array(
				array(array(Auth::instance(), 'hash'))
			)
		);
	}
[PHP] pobierz, plaintext 

Zakładam, że Twoje filtrowanie polega na usunięciu zbędnych znaków, pustych spacji, itd.