Code Bewerten

beat78 · May 8, 2013, 9:55am

Hallo Zusammen

Darf ich Euch fragen wie Ihr den Code findet? Mache ich das kompliziert, nicht korrekt oder ist das ok so?

Danke für das Feedback.

Controller


public function actionProject() {


        $data = new TblProjekt;

  $form = new TblStatus;

  $modelNachricht = new Tbl_Nachrichten;


        // Gast und $_GET ID prüfen 

        if (isset($_GET['id']) && !Yii::app()->user->isGuest) {


            $data = TblProjekt::model()->findAllByPk($_GET['id']);

   

        } else {

            $data = TblProjekt::model()->findAll();

        }

  

  // Wenn der Benutzer Gast ist und sich bewirbt

        if (Yii::app()->user->isGuest) {


            $msg = "Bitte zuerst anmelden.";

            $this->render('mobile', array('var1' => $msg, 'typ' => 'alert alert-error', 'data' => $data));  

   

        } else {

  

   if (isset($_POST['TblStatus'])) { 

  

    // Wenn der Benutzer angemeldet ist und sich bewirbt

    $form->setAttributes($_POST['TblStatus']);

    $modelNachricht->setAttributes($_POST['Tbl_Nachrichten']);

    $modelNachricht->idfs_User = Yii::app()->user->id;

    $modelNachricht->idfs_Projekt = $_GET['id'];

    $modelNachricht->save();

    $form->idfs_Bewerber = Yii::app()->user->id;

    $form->idfs_Faehigkeit = $form->idfs_Faehigkeit;

    $form->idfs_Projekt = $_GET['id'];

    $form->Datum = time();

    $form->Status = "Anfrage offen";

    $form->Save();

    

    

     if ($form->validate()) {

     

     // Mail versenden

     $Projekt = TblProjekt::model()->findByPK($_GET['id']);

     $User = User::model()->findByPK($Projekt->Benutzer);

     $AnbieterEmail = $User->username;

     Yii::app()->Mail->Send_Bewerbung($AnbieterEmail, $Projekt);

     

     $msg = "Die Bewerbung wurde erfolgreich an den Projektleiter versendet.";

     $data = TblProjekt::model()->findAll();

     $this->render('mobile', array('var1' => $msg, 'typ' => 'alert alert-success', 'data' => $data,'model'=>$form, 'modelNachricht'=>$modelNachricht));

     }

      

   } else {

   

   // Wenn Formular leer ist

   

   $msg = "Bitte wählen Sie die Fähigkeit aus";

   $this->render('mobile', array('var1' => $msg, 'typ' => 'alert alert-info', 'data' => $data,'model'=>$form, 'modelNachricht'=>$modelNachricht));

   }

    }

 }

View


<div> <?

  if (isset($var1)) {

   Echo "<div class='". $typ ."'>";

   echo $var1;

   echo "</div>";

  } ?>

 </div>


 <?

 

 // Wenn es um eine Bewerbung geht

 if( isset($model)) {

  

  // Pinboard anzeigen

  Yii::app()->Pin->CreatePin($data); 

  

  // Fähigkeiten auswählen

  $this->renderPartial('myform', array('model' => $model, 'modelNachricht' => $modelNachricht));  

  

 } else {

 

  // Pinboard erstellen

  Yii::app()->Pin->CreatePin($data);

 }


 $model = TblProjekt::model()->FindByPk(29); ?>

SleepWalker · May 9, 2013, 4:48pm

Hallo beat,

diese Zeile brauchst du nicht:


$data = new TblProjekt;

du kannst auch anstatt


...

}

else

{

  if(...) 

  {

  ...

  }

  else

  {

  ...

  }

}

so schreiben:


...

}

elseif(...) 

{

  ...

}

else

{

  ...

}

vielleicht diese Zeile ist sinnlos?


$form->idfs_Faehigkeit = $form->idfs_Faehigkeit;

genauso wie diese:


$model = TblProjekt::model()->FindByPk(29);

Mailversand würde ich als ein Method von TblProjekt Model implementieren.

Du könntest auch weniger Verzweigungen machen mit Benutzung von Yii::app()->end() nach der render() Function.

Falls du willst, du könntest Anhand der Rules den Gastzugriff begrenzen (Ich weiß aber nicht, ob du deine eigene Nachricht einstellen kannst).

Meiner Meinung nach, $var1 ist unverständliche Name für Variable.

Nachrichten wie ‘alert’ könnte man mit Yii::app()->user->setFlash() in gesamte Seite Layout integrieren. Wenn du würdest so machen, dann wird es vielleicht für dich ein render() Funktion pro actionProject Method genug!

P.S. alle diese Tipps es ist nur wie es implementieren könnte. Das bedeutet nicht, dass jeder so tun muss. Jeder hat sein eigenen Codestil. Es kann aber sein, dass in mein Post etwas eintreten wird, dass du nicht über Yii weißt.

BG

Sviat

mikl · May 10, 2013, 7:42am

Da du explizit danach fragst: Ich hab bereits nach wenigen Zeilen aufgehört, deinen Code zu lesen, weil die Einrückungen (Indentation) nicht passen. Das erschwert das Lesen für andere ungemein. Daher als Tipp: Gewöhn dir das an. Sauber eingerückter Code macht es nicht nur dir, sondern auch anderen leichter.

Siehe auch hier: http://de.wikipedia.org/wiki/Einrückungsstil

mbi · May 10, 2013, 11:43am

weitere Stichworte: Coding Style, Coding Standard, Konventionen

zum Beispiel TblProjekt vs Tbl_Nachrichten (einerseite Singular/Plural, andererseits PrefixEntity/PrefixUnderscoreEntity)

codesprache englisch vs landessprache (Nachricht vs User, Yii::app()->mail->Send_Bewerbung())

…

beat78 · May 12, 2013, 8:51pm

Hallo Zusammen

Vielen Dank für die wertvollen Tipp’s. Habe die Konventionen überarbeitet und die Tipp’s umgesetzt.

Das ich mit meiner doch eher geringen Erfahrung noch keinen Preis gewinne bin ich mir bewusst. [size="2"]Darf ich nochmals Fragen ob der Code so im Durchschnitt der Lesbarkeit und Wartbarkeit liegt oder liegen noch Grundlegende Fehler vor?[/size]


    public function actionCreateProjektTeilnehmen() {


        $modelStatus = new Tbl_Status;

        $modelNachricht = new Tbl_Nachricht;


        // Prüfen ob Gast und filtern 

        if (isset($_GET['id']) && !Yii::app()->user->isGuest) {


            $data = Tbl_Projekt::model()->findAllByPk($_GET['id']);

        } else {

            $data = Tbl_Projekt::model()->findAll();

        }


        // Prüfen ob Member

        if (Yii::app()->user->isGuest) {


            $msg = "Bitte zuerst anmelden.";  // 

            Yii::app()->user->setFlash('leer', $msg);

            $this->render('Pinnwand', array('data' => $data));

        } else {


            if (isset($_POST['Tbl_Status'])) {


                // Nachricht eintragen

                $modelNachricht->setAttributes($_POST['Tbl_Nachricht']);

                $modelNachricht->idfs_User = Yii::app()->user->id;

                $modelNachricht->idfs_Projekt = $_GET['id'];




                // Status eintragen

                $modelStatus->setAttributes($_POST['Tbl_Status']);

                $modelStatus->idfs_Bewerber = Yii::app()->user->id;

                $modelStatus->idfs_Projekt = $_GET['id'];

                $modelStatus->Datum = time();

                $modelStatus->Status = "Anfrage offen";


                // Formular Verarbeitung

                if ($modelStatus->validate() && $modelNachricht->validate()) {


                    $modelNachricht->save();

                    $modelStatus->Save();


                    // Mail versenden

                    $Projekt = Tbl_Projekt::model()->findByPK($_GET['id']);

                    $User = User::model()->findByPK($Projekt->Benutzer);

                    $AnbieterEmail = $User->username;

                    $this->Send_Bewerbung($AnbieterEmail, $Projekt);


                    // Information

                    $msg = "Die Bewerbung wurde erfolgreich an den Projektleiter versendet.";

                    Yii::app()->user->setFlash('leer', $msg);

                    $data = Tbl_Projekt::model()->findAll();

                    $this->render('Pinnwand', array('data' => $data, 'modelStatus' => $modelStatus, 'modelNachricht' => $modelNachricht));

                }

            }


            // Formular ist noch leer


            $msg = "Bitte wählen Sie die Fähigkeit aus";

            Yii::app()->user->setFlash('leer', $msg);


            $this->render('Pinnwand', array('data' => $data, 'modelStatus' => $modelStatus, 'modelNachricht' => $modelNachricht));

        }

    }


<?php


// Pinwand Zeigen

Yii::app()->Pin->PinnwandZeigen($data);


if (isset($modelStatus)) {


    // Fähigkeiten auswählen

    $this->renderPartial('myform', array('modelStatus' => $modelStatus, 'modelNachricht' => $modelNachricht));

}

?>


<div class="form">


    <?php

    $form = $this->beginWidget('CActiveForm', array(

        'id' => 'tbl-status-status-form',

        'enableClientValidation' => true,

        'enableAjaxValidation' => false,

        'clientOptions' => array(

            'validateOnSubmit' => false,

        ),

        'htmlOptions' => array(

            'enctype' => 'multipart/form-data',

        ),

    ));

    ?>


    <?php //echo $form->errorSummary($modelStatus); ?>


    <div class="row">

        <?php echo $form->labelEx($modelStatus, 'F&auml;higkeit ausw&auml;hlen'); ?>

        <?php echo $form->dropDownList($modelStatus, 'idfs_Faehigkeit', CHtml::listData(Tbl_Faehigkeit::model()->findAll(), 'primary', 'Faehigkeit')); ?>

        <?php echo $form->error($modelStatus, 'idfs_Faehigkeit'); ?>


        <?php echo $form->labelEx($modelNachricht, 'Nachricht Initiant'); ?>

        <?php echo $form->textArea($modelNachricht, 'Nachricht', array('size' => 10, 'maxlength' => 50, 'style' => 'height:200px;')); ?>

        <?php echo $form->error($modelNachricht, 'Nachricht'); ?><br>

        <?php echo CHtml::submitButton('Bewerben'); ?>


    </div>


    <?php $this->endWidget(); ?>


</div><!-- form -->

mikl · May 13, 2013, 6:45am

Sieht doch gleich viel lesbarer aus .

Die Logik sieht auch o.k. aus, soweit ich die gewünschte Funktion erahnen kann. Du hast allerdings zwei mal die Prüfung isGuest drin. Wenn du die Models erst im else-Block von "Prüfen ob Member" weiter unten lädst, kannst du das erste "&& !Yii::app()->user->isGuest" eliminieren.

beat78 · May 13, 2013, 5:45pm

Hallo Mike,

Vielen Dank für das Feedback und den erneuten Tipp.

Gruss Beat