2
\$\begingroup\$

I have question about scope yesterday, and someone gives an answer to don't return anything in scope. Is return something in scope really bad practice? Should I not return anything in the model except query?

Rather than scope, I want to create a normal method to return the select2 format. I have so many model/table that I need to return to select2 format, I think it's WET (write every time) if I must write every model or controller. I think I need to use DRY(Don't Repeat Yourself) principle, so I want to use a trait like below:

trait ModelTrait
{
    public static function getSelect2format($column = "name")
    {
        $data = static::select('id', $column.' as text')->orderBy($column, 'asc')->where($column, "like", "%".request()->q."%");

        if (is_array(request()->filter) && !empty(request()->filter)) {
            foreach(request()->filter as $key => $val)
            {
                if(in_array($key,$this->filterable))
                {
                    $data = $data->where($key, $val);
                }

            }
        }
        $data      = $data->limit(5)->get();

        return $data;
    }
}

I just need to call use ModelTrait in every model that I need and call it in the controller like below:

public function selectJson()
{

    $data = Customer::getSelect2Format("column_name");


    return \Response::json($data);
}

Is the code above break the best practice? What are the concerns or something that I need to improve?

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I'm sorry to inform you that your code is not working as intended. You cannot call a non-static method statically. Well actualy you can, but you will get a fatal error if the method uses $this. Your method uses $this on the very first line of its body. Not working code is off-topic on this site. \$\endgroup\$ – slepic Jun 18 '20 at 4:35
  • 1
    \$\begingroup\$ @slepic Sorry, Actually I have not tested it. Now I tested it and it's working, just changed the method type to static and $this to static:: , Hope you can improve more, Thank you. \$\endgroup\$ – Muhammad Dyas Yaskur Jun 18 '20 at 5:42
3
\$\begingroup\$

I have question about scope yesterday, and someone gives an answer to don't return anything in scope. Is return something in scope really bad practice?

  • This is because laravel is handling "it" for you.

  • getSelect2Format is not the best name since you are not handling formatting but apply a specific set of rules relevant to select2 js plugin.

  • I would suggest a repository class or a helper class to handle this task.

  • Your code is messy, very long lines too, which is hard to read! I would suggest following PSR-12.

  • I would suggest using response() or just return $data in controller class methods.

  • I would also suggest early returns in your methods to reduce nesting hence easy reviewing by reviewers.

  • suggest not using ->get() in a method unless you are absolutely certain on what method should do. i.e. you might want to apply a scope to this query later on.

  • I suggest passing request data into the class to make it easier to test. Also, the less your class knows about other parts of the system the better.

  • Dont you have to use DB::raw for $column.' as text'?

class select2Filters
{
    public static function apply(
        Model $model, 
        array $data, 
        string $column = 'name', 
        int $limit = 5
    ) {
        $filter = Arr::get($data, 'filter');
    
        return $model::selectRaw(['id', "{$column} as text"])
             ->where($column, 'like', "%{$data['q']}%")
             ->when(is_array($filter), function($query) use ($filter) {
                 $query->where(Arr::only($filter, $model::filterable)); 
             })
             ->orderBy($column, 'asc')
             ->limit($limit)
             ->get();
    }
}

I would also suggest adding an interface to each model that should have support for select2Filters. That interface should inforce filterable variable.

I would personally not use static, that way you can add configuration methods, i.e. setLimit(). OR use data struct class to contain data needed for select2Filters class.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your review, I like it. And sorry, my code was written when I write this question(without any ide/editor) so it's not formatted correctly. Usually, I use auto formatted to PSR12 using PhpStrom code formatted. And actually, I just created my own library for this case and also to learn or improve my skill, maybe I will create a question about that library. About PSR12, I have another question which maybe you can help stackoverflow.com/questions/62480588/… \$\endgroup\$ – Muhammad Dyas Yaskur Jun 21 '20 at 12:17
  • \$\begingroup\$ Hi, I am afraid I have no idea on how to handle formatting in php storm, from what I know you meant to have a config which defines formatting and that configuration can be downloaded from the official psr website \$\endgroup\$ – user3402600 Jun 21 '20 at 15:02

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.