2
\$\begingroup\$

I wrote a small class that does a database lookup and it generates a collection/Array. The Collection is resorted based on values returned from the database. Finally, the records can be retrieved using the public method called getQuestions().

I trying to move away from procedural code into object-oriented programming. I am trying to learn OOP "the right way" and I want to make sure my code met all the standards that exists for a reason.

I understand that there is multiple way to get the any done. But I really want to learn the better way.

  1. Is it documented enough/correctly?
  2. Does it meet the single responsibility principle?
  3. Does it meet the open/closed principle?
  4. Is there another principle that it is violating?
  5. What is a better way of re-writing this class?

<?php namespace App\Modules\Surveys\Classes\Questions\Surveys;

use App\Modules\Surveys\Models\SurveyPage;

use App\Modules\Surveys\Models\SurveyQuestion;

class GenerateNewQuestions
{

    protected $currentPageId;

    protected $questions;

    /**
     * Create a new instance.
     *
     * @return void
     */
    public function __construct($currentPageId)
    {
        $this->currentPageId = $currentPageId;

        $this->handle();
    }

    /**
     * Execute the job.
     *
     * @return void
     */
    protected function handle()
    {

        $page = $this->getPage();

        $questionsQuery = $this->buildBaseQuery( $page->survey_id, $page->id );

        if( $page->sort_questions_by == 'Defined Sort'){
            //Order by the value in the "sort" property

            $this->handleDefinedSort( $questionsQuery, $page->sort_questions_direction );
        }


        if( $page->max_questions ){
            //set limit of total questions to a max value

            $questionsQuery->take( $page->max_questions );
        }

        //Get the new questions collection
        $newQuestions = $this->executeQuery( $questionsQuery );

        if( $this->shouldShuffleQuestions( $page->sort_questions_by ) ){
            //Shuffle the questions, if questions should be shuffled

             $newQuestions->shuffle();
        }

        if( $page->sort_questions_by == 'Defined And Random'){
            //Sort the question based on the value of "sort", then shuffle when sort is null
            $this->handleDefinedAndRand( $newQuestions );
        }

        $this->setQuestions( $newQuestions );

    }

    /**
    * It gets the current questions
    * 
    * @return \Illuminate\Support\Collection
    */
    public function getQuestions()
    {
        return $this->questions;
    }

    /**
    * It gets the current page
    * 
    * @return \Illuminate\Support\Collection
    */
    protected function getPage()
    {
        return SurveyPage::findOrFail( $this->currentPageId );
    }

   /**
    * It sets the value of the current questions
    * 
    * @return void
    */
    protected function setQuestions($questions)
    {
        $this->questions = $questions;
    }


   /**
    * It check whether the questions should be shuffled or not
    *
    * @param  string $sortBy
    * @return boolean
    */
    protected function shouldShuffleQuestions( $sortBy )
    {
        return $sortBy == 'Random' ? true : false;
    }


   /**
    * It sorts the collection by the "sort" value if is is not null, then it will shuffle the questions where the sort is null
    * 
    * @param newQuestions \Illuminate\Support\Collection
    * @return void
    */
    protected function handleDefinedAndRand(&$newQuestions)
    {

        $newQuestions->sort( function ($a, $b){

            if( is_null($a->sort) ){
                return -1;
            }

            if( $a->sort === $b->sort){
                return 0;
            }

            return $a->sort < $b->sort ? -1 : 1;

        });


    }


   /**
    * It generated the base query
    *
    * @param Integer $surveyId
    * @param  Integer $pageId
    * @return \Illuminate\Database\Eloquent\Builder
    */
    protected function buildBaseQuery($surveyId, $pageId)
    {

        return SurveyQuestion::surveyAndPageAndStatus($surveyId, $pageId)
                                   ->with([
                                            'controls' => function($query){
                                                    //Ordering by parent_control_id, parent_item_index in ASC order is very important
                                                    //It will allow the regorganize function to first work the
                                                    //parent controls, control children, and finally the parent_item_index
                                                    $query->activeControls()->orderBy('parent_control_id', 'asc')->orderBy('parent_item_index', 'asc');
                                            },
                                            'controls.items' => function($query){
                                                $query->activeItems()->orderBy('sort')->orderBy('title');
                                            }
                                           ]);

    }

    /**
    * It sorts the query builder using a direction
    *
    * @param  String $sortDirection: it should be wither 'asc' or 'desc'
    * @return void
    */
    protected function handleDefinedSort(&$query, $sortDirection = 'asc')
    {
        if( $sortDirection != 'asc'){
            //Make sure the valus is set to "desc" if it is not "asc"
            $sortDirection = 'desc';
        }

        $query->orderBy('sort', $sortDirection);
    }


    /**
    * It executes a giving query
    * 
    * @param $query \Illuminate\Database\Eloquent\Builder
    * @return \Illuminate\Support\Collection
    */
    protected function executeQuery($query)
    {
        return $query->get();
    }

}

Here is an example on how I would call this job from somewhere else in my application:

$GenerateNewQuestions = new GenerateNewQuestions( 20 );

//Do something with the questions generated by the GenerateNewQuestions class
dd( $GenerateNewQuestions->getQuestions()  );

This is no longer a job event, rather a small class that does one thing only.

\$\endgroup\$
  • \$\begingroup\$ I'll write up something when I get home from work but just looking at the code I can say that you need to treat the job as a queue, and should be accessing the results through an event listener. The benefit of this method is that you could queue up hundreds into say Redis, and have them process async without effecting the main app. By making the job only work synchronously you are breaking the main use of a Job and should probably use a different design pattern. \$\endgroup\$ – Mike Nov 17 '15 at 21:47
  • \$\begingroup\$ @Mike please check my question once again. I changed this from a Job into a standard class. I learned from another source that there is not point in using Job here. \$\endgroup\$ – Junior Nov 18 '15 at 0:36
1
\$\begingroup\$

Your code is good so far. kindly check below suggestions.

About Doc comments for the variables

/**
 * 
 * @var integer
 */
protected $currentPageId;

/**
 * 
 * 
 * @var array
 */
protected $questions;

About writing if conditions

Whenever you are comparing hard coded values in if condition then you have to place this hard-coded values at left hand site to avoid mistakenly variable assignments.

if( $page->sort_questions_by == 'Defined And Random')

As this will changed as

if( 'Defined And Random' == $page->sort_questions_by)

About using Hard-coded strings

No restriction on comparing hard code strings in your if condition, but better way is to assigned that value to class constants.

The reason behind this to do so , Suppose you have 1000 of lines of code and you had used this kind of hard coded string value for comparison or any other purpose every where in your code.

If in future you supposed to change the value of that hard coded string then you have to make changes everywhere in your code. So to avoid this you can do as follow by defining class constants.

/**
 * 
 * @var string
 */
const DEFINED_SORT = 'Defined Sort';

/**
 * 
 * @var string
 */
const DEFINED_AND_RANDOM_SORT = 'Defined And Random';

/**
 * 
 * @var string
 */
const SORT_BY = 'Random';

/**
 * 
 * @var string
 */
const SORT_DIRECTION_ASC = 'ASC';

/**
 * 
 * @var string
 */
const SORT_DIRECTION_DESC = 'DESC';

So now your if condition will change as :

if( GenerateNewQuestions::DEFINED_AND_RANDOM_SORT == $page->sort_questions_by)

or

if( self::DEFINED_AND_RANDOM_SORT == $page->sort_questions_by)

This is good option because even if you miss single = from the if condition then it will try to assign value to defined constant it will throw an PHP E_ERROR that you are trying to change the value of constant.

One logical mistake that I can see in your code

The function getPage will return the null on fail and on success it will give you the SurveyPage object. In handle function you have assign the result of getPage function to $page variable.

Here in handle function you have to handle the condition if the $page is NULL. Because if it is NULL then at if( $page->sort_questions_by) this line it will give an ERROR as Trying to access sort_questions_by property of undefined object or null (May be with different message)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.