2014-08-31 79 views
0

对于cakephp来说,我非常新,但我从一开始就喜欢它。经过几小时的研究和教程,我现在觉得能够创建符合我的要求的User :: register()函数。该功能运行良好,但有些时候我不满意代码。而且,因为我想确保我的理解CakePHP的逻辑很好,我想问问你,如果那里有一个更好的方式来做到这一点,或者如果它的漂亮的代码(见注释):cakephp UsersController :: register()函数

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('[email protected]'.$_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'); 
     } 
    } 
} 

任何意见将是非常赞赏。

谢谢大家提前, oligen

回答

1

架构

移动电子邮件发送代码到一个单独的方法。好的OOP意味着一种方法只执行一项任务,您的注册方法也会处理电子邮件发送,将其分离为另一种方法并从您的注册方法中调用。为了能够单元测试这个更好,我通常会创建这样

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

的方法,很容易嘲笑这个方法,然后在测试中并返回嘲笑CakeEmail对象,并断言方法调用就可以了然后。

移动整个数据处理逻辑到应在何处(表示控制器调用模型法)模型层:不要

$errmsg, $vcode 

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

最佳初步实践使用简短的变量名称,这个是可以猜测的,但通常易于阅读和理解代码比缩短任何事情都好。那么为什么不把它叫做$ errorMessage和$ verficationCode呢?

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

if后缺少空格,follow the coding conventions。您可以使用像phpcs这样的工具来验证您的代码。

MVC违规,除了视图层以外,您从未想过在脚本中使用HTML。

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

<br>甚至没有在这里使用,使用CSS来创建元素之间的空间。 <br>换行符并没有被认为是滥用作为“设计元素”来创建间距。没有必要运行验证错误数组并添加<br>

如果遵循约定,CakePHP应该在每个输入字段下方自动显示错误消息,如果该字段的验证规则失败。

+0

嗨burzum,非常感谢你的详细评论!我会重做一些事情,也许再问你一遍,好吗? – oligen 2014-08-31 17:52:46

+0

欢迎,你介意将答案标记为正确吗?谢谢! – burzum 2014-08-31 18:08:13