Code Review

This is my first Yii project and so wanted someone to do a code review. Any suggestion welcome

Controller


<?php


class VerifyUsersController extends Controller

{

	/**

	 * @param   string 	$param 			key to be checked		

	 * @return  boolean 	$YiiResult 		weather data was saved or not.

	 */

	public function actionAccount($param = null)

	{

		$message = array(

				'type' 		=>'error', 

				'message'	=>'Invalid URL.');


		if($param != null){


			$VerifyUser 	= new VerifyUser();

			if($VerifyUser->readByKey($param) !== false and $VerifyUser->status == 'active'){


				$User = new User();

				$User->id = $VerifyUser->user_id;


				if(($User->get() and $User->status == 'active') || 

				   ($User->status == 'new' and $User->changeStatus('active')))

				{

					$VerifyUser->changeStatus('deleted');

					$message = array(

							'type' 		=>'success', 

							'message'	=>'Account verified.');

				}

				else{

					$message = array(

							'type' 		=>'error', 

							'message'	=>'Verification failed');

				}

			}

		}	


		$model = new LoginForm;


		$this->render('//site/login',array('message'=>$message, 'model' => $model));

	}


	public function newUser(){

                $user=new User('signup');

		if(isset($_POST['User']))

		{

			$_POST['User']['user_type_id'] = Yii::app()->params['userType']['normal'];

			$user->attributes=$_POST['User'];


			if($user->validate() and $user->create()){

				$model = new VerifyUser;

				$model->attributes = array( 'user_id' => $user->id,

					'data' => $user->key,

					'data' => $user->email,

					'type' => 'account' );

				return $model->create();

			}

		}

}

?>

Model


<?php


class VerifyUser extends CActiveRecord

{

	/**

	 * Returns the static model of the specified AR class.

	 * @param string $className active record class name.

	 * @return VerifyUser the static model class

	 */

	public static function model($className=__CLASS__)

	{

		return parent::model($className);

	}


	/**

	 * @return string the associated database table name

	 */

	public function tableName()

	{

		return '{{verify_users}}';

	}


	/**

	 * @return array validation rules for model attributes.

	 */

	public function rules()

	{

		// NOTE: you should only define rules for those attributes that

		// will receive user inputs.

		return array(

			array('id, user_id, key, type', 'required'),

			array('id, user_id', 'numerical', 'integerOnly'=>true),

			array('key', 'length', 'max'=>200),

			array('data', 'length', 'max'=>255),

			array('type', 'length', 'max'=><img src='http://www.yiiframework.com/forum/public/style_emoticons/default/cool.gif' class='bbc_emoticon' alt='8)' />,

			array('created', 'safe'),

			// The following rule is used by search().

			// Please remove those attributes that should not be searched.

			array('id, user_id, key, data, type, created', 'safe', 'on'=>'search'),

		);

	}

       /**

	 * @return array relational rules.

	 */

	public function relations()

	{

		// NOTE: you may need to adjust the relation name and the related

		// class name for the relations automatically generated below.

		return array(

		);

	}


	/**

	 * @return array customized attribute labels (name=>label)

	 */

	public function attributeLabels()

	{

		return array(

			'id' => 'ID',

			'user_id' => 'User',

			'key' => 'Key',

			'data' => 'Data',

			'type' => 'Type',

			'created' => 'Created',

		);

	}


	/**

	 * Genetate a key.

	 * @return 	boolean 	$YiiResult 	weather data was saved or not.

	 */

	public function create(){

		

		$YiiResult = Yii::app()->db->createCommand()

					->insert('{{verify_users}}', array(

						'user_id' 	=> $this->user_id,

						'key' 		=> $this->key,

						'data' 		=> $this->data,

						'type' 		=> $this->type

				));

		return $YiiResult;


	}


	/**

	 * Read a key.

	 * @param   string 		$key 			key to be checked		

	 * @return 	mixed 	 	$YiiResult 		result or false.

	 */

	public function readByKey($key){

		$YiiResult = Yii::app()->db->createCommand()

					->select('*')

					->from($this->tableName())

					->where('`key`=:key', array(':key'=>$key))

					->queryRow();

		if($YiiResult == true){

			$this->setAttributes($YiiResult, false);

		}

		return $YiiResult;

	}


	/**

	 * Change Key Status.

	 * @param 	string 		$status 		user status

	 * @return 	boolean 	$YiiResult 		result or false.

	 */

	public function changeStatus($status){

		if($this->id == false){

			return false;

		}


		$YiiResult = Yii::app()->db->createCommand()

					->update($this->tableName(),array(

							'status'=>$status

					), 'id=:id', array(':id'=>$this->id));


		return ($YiiResult == 0) ? false : true ;

	}

}











<?php


class User extends CActiveRecord

{


...................

...................

...................

...................

	/**

	 * Create a new user.

	 * @return 	boolean 	$YiiResult		weather data was saved or not.

	 */

	public function create(){

		$YiiResult = Yii::app()->db->createCommand()

					->insert($this->tableName(), array(

						'username' => $this->username,

						'email' => $this->email,

						'password' => $this->password,

						'user_type_id' => $this->user_type_id,

						'status' => 'new',

				));

		if($YiiResult == true){

			$this->id = Yii::app()->db->getLastInsertID();

		}

		return $YiiResult;


	}





}

?>

Well, this might sound a bit cruel and/or harsh, but my old friend beer just reassured me this were absolutely justified:

[color="#8b0000"]<moderator>Video removed</moderator>[/color]

Please follow these steps:

  • Delete what you created
  • Read the blog tutorial and possibly the Definite Guid to Yii (again)
  • Start anew

Could you point out a few mistakes. I did not want to use active records coz they are slower.

Hm, well. I think you took the stanza about performance and AR a bit too serious. While Active Records are slower (especially in batch operations), they are great to speed up your development. Actually, this is the point of most frameworks ;)

As for your code: I think you really missed the point on how controllers, models and views are working together in Yii (or MVC in general). Although it seems like you’ve really embraced the “Fat-Model Approach,” it looks like you embraced it a bit too much. Methods like VerifyUser.changeStatus() suggest that. VerifyUser.readByKey() is also bogus: VerifyUser.findByPk() (inherited from CActiveRecord) should already take cae of that. Also: The name VerifyUser is a bit odd and suggests business logic that were better contained in model rules. If you could tell a bit more about your specific use case, I might be able to provide better advice.

Good, this was more helpfull than the previous one. Yes i prefer the fat-model or quires in model only approach.

I am building a website for speed and there is AR bashing in most places when talking about speed.

The code above does this. When a user signs up, a entry is made in the verify users table and a key is generated. This key is sent in email as link… When user clicks on it. It changes account status also status deletes the key.

Well, I think you’re really optimizing at the wrong end. It sounds to me like the verify_users table isn’t needed at all. I think the verification code could easily be contained in a field in your user table. Saying that, all of the code for verification and status update should really belong into your User class.

Here’s what I would do:

  • Have a set of flags for the user statuses defined as class constants (class constants are not as expensive as defines, but they are immensely useful to minimize typing errors etc)

  • Set a unique verification code upon model creation in User.beforeSafe() (just check if $this->isNewRecord is true and set $this->verification_code. Do not forget to call parent::beforeSave().

  • Create two new controller actions in the UserController: actionSignup() and actionVerify($code). In actionVerify(), I’d simply check if the given code equals the verification code in the DB and set the user’s status to verified if they match. No need to delete the code. But of course, you are free to NULL the field :)

Thanks for all the help. You are exactly right for just signup i dont need another table. But I used another table cos this will be used to verify all sort of things from forgot password to new user signup to email change etc.

For now lets assume logically this is what we want to do, could you point out all the yii mistakes that i am doing. form a pure yii standpoint what all changes do you recommend

Could you help me on when to use active record and when not.

Your code is not that awful, the first response was entirely uncalled for.

For the simple queries that you have demonstrated, AR is perfect. You will never ever notice the microsecond difference with using DAO.

For complex queries, DAO is recommended (in the docs) - use a query debugger if you are unsure.

Adding to that: Try to find the "hot zones" of your webapp. That are those where you would want to optimize because they will be pulled frequently. Optimizing the signup process or verification codes makes little sense (at least as long as they are loading within acceptable amounts of time). And actually, Active Records can be quite expensive as AR models are loading all table fields belonging to a record by default. You can work around that by setting scsopes in place that will only pull certain fields into the result set. OTOH, AR models can also be great for performance improvment: Using relational queries, they can be quite useful to get a set of model and a number of related ones - all through one constructed query. If it really helps performance depends on your webapp, though. Just give it a try. If the resulting query turns out to be too complex, fall back to DAO as Backslider wrote.

Both, AR and DAO have quite convinient access to query caching, which will probably help a lot more improving performance than the tricks above. If you have to deal with a lot of fat models on one pache and you cannot make use of query or data caching (let it be that this would tax the cache too much or you cannot risk serving possibly stale data from the cache), DAO would be the perfect choice.

I know optimising signup or verification process is un-called for… i just wanted a basic yii review of the code. I just wnat to know if there are any yii mistakes i am doing