4
\$\begingroup\$

I'm using PHP (Laravel) to iterate over the survey_answers table and build datasets which I can show in a graph. I'm just wondering if this is the way to go.

ChartFactory.php

public static function buildFromSurvey($survey)
{
    $chart = new Chart($survey->name);

    foreach($survey->answers as $answer) {

        if($chart->hasDataset($answer->user_id)) {
            $dataset = $chart->getDataset($answer->user_id);
        } else {
            $dataset = new Dataset();
            $dataset->setTitle($answer->user->name);
            $dataset->setColor(config('assessment.chart_colors_survey')[0]);
            $dataset->setOwner($answer->user);
            $chart->addDataSet($dataset);
        }

        // Create the category if not exists in this dataset
        if ($dataset->hasDataItem($answer->question->category->name)) {
            $category = $dataset->getDataItem($answer->question->category->name);
            $category->addScore($answer->score);
        } else {
            $category = new Category();
            $category->setName($answer->question->category->name);
            $category->addScore($answer->score);

            $dataset->addDataItem($category);
            $chart->addLabel($answer->question->category->name);
        }
    }

    return $chart;
}
\$\endgroup\$
2
\$\begingroup\$

I think my main criticism of this code would be that it is asymmetric in terms of where ownership of instantiating your different object dependencies lives.

For example, in the first conditional around whether Dataset exists on the Chart, why does this code need to own the logic to create and attach a new Dataset? If the Chart object "owns" the datasets, I would think that perhaps all this code would do is call a Chart::createNewDataset() or similar method to handle the work building a new dataset and setting it on the Chart.

This same concern exists for me with regards to creating Category objects.

I would also strongly recommend using type hinting for the parameter to enforce that you are getting passed a valid survey object.

\$\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.