Proper Code

Hi guys, I’m creating an application which lets you submit ratings (for some reason I called it rankings) on different videogames on different platforms. You can have one rating per platform per game, and you can update your existing ratings. The final product looks like this:

4254

Skærmbillede 2013-05-15 kl. 14.34.17.png

The thing is that even though the system works, I suspect it does not follow the Yii way of doing things. I don’t know if this could hurt the performance or the security of the site, or if it is only for the purpose of having code that’s easy to read and change?

I’m really having trouble understanding how it all should be written properly even though I’ve read two books on Yii and “The Guide”.

Can you please look through my code and tell me how it should be made if I should follow the Yii way of doing it?

views/ranking/_form:


<?php echo $form->hiddenField($model, 'platform_id', array('value' => $key)); ?>

	

	<?php $x="platform_id=$key"; ?>

    

    <div class="overall">

		<h4>Overall</h4>

        <h3><?php echo round($game->overallAVG(array('condition'=>$x)),1) ;?></h3>

        <?php $this->widget('CStarRating',array(

			'model'=>$model,

			'attribute' => 'overall',

		)); ?>

	  <?php echo $form->error($model,'overall'); ?>

    </div>

    

  <div class="ranking-left">

		<p>Graphics</p>

        <p style="font-size:18px">

          <?php $this->widget('CStarRating',array(

			'model'=>$model,

			'attribute' => 'graphics',

		)); ?>

          <?php echo $form->error($model,'graphics'); ?>

        &nbsp; <?php echo round($game->graphicsAVG(array('condition'=>$x)),1) ;?></p>     

    </div>

    

 ... more of the same ...


	<div class="row buttons">

		<?php echo CHtml::submitButton($model->isNewRecord ? 'Submit ratings' : 'Save ratings'); ?>

	</div>

views/game/view:


<?php 

			$tab_list=Platform::getPlatforms();

			$tabarray=array();

 

			// Create Dynamic Tabs

			foreach($tab_list as $key=>$value){

				$tabarray["$value"]=array(

					'id'=>$key,

					'content'=>$this->renderPartial('../ranking/_form',

					array('model'=>$ranking[$key], 'key'=>$key, 'game'=>$model),TRUE)

				);

			}?>


			<?php

			$this->widget('zii.widgets.jui.CJuiTabs',array(

				'tabs'=>$tabarray,

				'options'=>array(

					'collapsible'=>false,

				),

				'id'=>'categorytabs',

				'themeUrl'=> Yii::app()->baseUrl . '/css/game/tabs',

				'cssFile'=>'jquery-ui.css', 

			)); ?>

controllers/gameController:


    public function actionView($id)

	{

		if(!Yii::app()->User->checkAccess('game:view'))

		{

			throw new CHttpException(401);

		}

		

		$model=$this->loadModel($id);

		$genre=Genre::model()->findByPk($model->id);

		$platform=Platform::model()->findByPk($model->id);

		$sysReq=SysReq::model()->findByPk($model->id);

		$ranking=$this->getRankingList($model);

		

		$this->render('view',array(

			'model'=>$model,

			'genre'=>$genre,

			'platform'=>$platform,

			'sysReq'=>$sysReq,

			'ranking'=>$ranking,

		));

	}


... more code ...


    protected function getRankingList($model)

    {

        

        $user_id=Yii::app()->user->getId(); 

		$game_id=$model->id;

        $tab_list=Platform::getPlatforms();

        $rankings = array();

        foreach($tab_list as $key=>$value)

        {

            if(Ranking::model()->find("create_user_id=$user_id and game_id=$game_id and platform_id=$key"))

            {

            	$rank=Ranking::model()->find("create_user_id=$user_id and game_id=$game_id and platform_id=$key");

            } 

            else

            {

                 $rank = new Ranking;

            }

            $rankings[$key]= $rank;

        }

        

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

		{

			$platform = $_POST['Ranking']['platform_id'];

			$rankings[$platform]->game_id=$model->id;

			$rankings[$platform]->attributes=$_POST['Ranking'];

			//$scenario = $model->getScenario();


			if ($rankings[$platform]->save())

			{

			    //echo $scenario;

			    $this->redirect(array('view','id'=>$model->id));

			    //echo $platform;

			}

		}

        return $rankings;

    }



I’ve not had a thorough look through it, but here are a couple of suggestions.




            if(Ranking::model()->find("create_user_id=$user_id and game_id=$game_id and platform_id=$key"))

            {

                $rank=Ranking::model()->find("create_user_id=$user_id and game_id=$game_id and platform_id=$key");

            } 

            else

            {

                 $rank = new Ranking;

            }

            $rankings[$key]= $rank;



might be better as:




            $rank=Ranking::model()->findByAttributes(array(

                'create_user_id'=>$user_id,

                'game_id'=>$game_id,

                'platform_id'=>$key,

            ));


            if ($rank === null)

                $rank = new Ranking;


            $rankings[$key] = $rank;



That will save you from performing the query twice.

Another suggestion I would make is changing this:




                        $platform = $_POST['Ranking']['platform_id'];

                        $rankings[$platform]->game_id=$model->id;

                        $rankings[$platform]->attributes=$_POST['Ranking'];



to:




                        $platform = $_POST['Ranking']['platform_id'];

                        $rankings[$platform]->attributes=$_POST['Ranking'];

                        $rankings[$platform]->game_id=$model->id;



unless you want the bulk assignment to be able to override the game_id attribute.

Thank you for the quick and useful answer!

Maybe you could separate the controller function "getRankingList($model)" into two different functions; One to get the list, and one to create the database record?

I don’t know if this is a good idea, and i don’t know how to create that, but perhaps someone else could elaborate on that?