cakephp UsersController :: register（）函数

Question

Im pretty much new to cakephp, but I loved it from the beginning. After hours of research and tutorials I now felt able to create a User::register() function thats fits my requirements. The function works well, but at some points im not satisfied with the code. And as I want to make sure I understood cakephp's logic well, I would like to ask you if theres a better way to do it, or if its nice code (see comments):

public function register() {
    if ($this->request->is('post')) {
        //setting the data, so that it can be validated 
        $this->User->set($this->request->data);
        //go on if no error occurs    
        if($this->User->validates()) {        
            //creatin a verification code the user has to click in the confirmation email to activate his account
            //i think this is not solved very well, because i set the verification code in 2 variables. 
            //also: is there a better way to set the verification code to the user model than setting $this->request->data['User']['verification_code'] ?
            $vcode = $this->request->data['User']['verification_code'] = sha1($this->request->data['User']['email'].rand(0,100));
            //creating the record in database
            $this->User->create();
            //save, dont validate again
            if ($this->User->save($this->request->data, array('validate' => FALSE))) {
                //sending mail
                $Email = new CakeEmail();
                $Email->from(array('no-reply@'.$_SERVER['SERVER_NAME'] => $this->viewVars['appName']))
                    ->to($this->request->data['User']['email'])
                    ->subject(__('Deine Registrierung bei %s',$this->viewVars['appName']))
                    ->template('register_confirmation','default')
                    ->emailFormat('both')
                    ->viewVars(array(
                        'appName' => $this->viewVars['appName'],
                        'first_name' => $this->data['User']['first_name'],
                        'verificationLink' => Router::url(array('controller' => 'users', 'action' => 'verifyEmail'),true).'?vcode='.$vcode //with the verification code. heres where is need the $vcode variable again. 
                        //i thought it would be pretty ugly to write $this->request->data['User']['verification_code'] again
                    ));

                if($Email->send()) {
                    $this->Session->setFlash(__('Registrierung erfolgreich! Wir haben eine Bestätigungs-E-Mail an %s gesendet. Klicke auf den darinstehenden Link, um dein Konto zu aktivieren.',array('<strong>'.$this->request->data['User']['email'].'</strong>')),'flash_alert_success');  
                    return $this->redirect(array('controller' => 'users', 'action' => 'login'));
                }                     
                $this->Session->setFlash(__('Die E-mail konnte nicht versendet werden. Fordere eine neue Bestätigungs E-Mail an.'),'flash_alert_error');
            }
            $this->Session->setFlash(__('Die Registrierung ist fehlgeschlagen. Bitte versuche es erneut.'),'flash_alert_error');

        } else {
            //ist there a better way to catch and display the validation errors?
            $errmsg = __('%sDie Registrierung konnte nicht abgeschlossen werden:%s','<strong>','</strong><br />');
            foreach($this->User->validationErrors as $key => $val) {
                $errmsg .= __($val[0]).'<br />';
            }
            $this->Session->setFlash($errmsg,'flash_alert_error');
        }
    }
}

Any comments would be very appreciated.

Thank you all in advance, oligen

Answer 1

Architecture

Move the email sending code into a separate method. Good OOP means one method is only doing one task, your registration method deals with email sending as well, separate that into another method and call it from your registration method. To be able to unit test this better I usually create a method like this

public function getCakeEmail($config = null) {
    return new CakeEmail($config);
}

in my AppModel, it is easy to mock that method then in tests and return a mocked CakeEmail object and to assert the method calls on it then.

Move the whole data handling logic into the model layer where it should be (showing the controller calling the model method):

public function register() {
    if ($this->request->is('post')) {
        if ($this->User->register($this->request->post)) {
            //...
        } else {
            //...
        }
    }
}

Best pratice:

$errmsg, $vcode

Don't use short variable names, this one is guessable but usually easy to read and understand code is better than shortening anything. So why not calling it $errorMessage and $verficationCode?

if($this->User->validates()) {  

Missing space after the if, follow the coding conventions. You can use a tool like phpcs to validate your code.

MVC violation, you never ever want HTML in your scripts except in the view layer.

$errmsg .= __($val[0]).'<br />';

And the <br> isn't even used right here, use CSS to create space between elements. <br> is a line break and not thought to be abused as a "design element" to create spacing. There is no need to run of the validation errors array and add that <br>.

If you followed the conventions CakePHP should automatically show the error messages below each of the input fields if the validation rules for that field failed.