0
\$\begingroup\$

I'd like a general opinion on my piece of code (which does exactly what I want it to do). I feel like I'm using bad practices, like a DB query within a foreach loop, or grabbing the lowest value in a non-elegant way (sorting, then using the object with an index of 0). I'm also not sure on my usage of the DB queries inside my controller, instead of passing it to the model.

Note this is on Laravel v3:

public function get_predict()
{
    $champions = DB::table('champions')
        ->get(array('id','name','released','price_ip','price_rp'));

    $now = strtotime(date('Y-m-d'));

    foreach($champions as &$champion) {
        $id = $champion->id;
        $champion_sales = DB::table('sales')
            ->where('type','=','1')
            ->where('for','=',$id)
            ->get();
        $champion->sales = $champion_sales;

        foreach($champion->sales as &$sale) {
            $dateout = strtotime($sale->date_out);
            $sale->days_since = floor(($now - $dateout)/(60*60*24));
        }

        usort($champion->sales, function($b, $a) {
            return strcmp($b->days_since, $a->days_since);
        });

        if(isset($champion->sales[0]->days_since)) {
            $champion->most_recent = $champion->sales[0]->days_since;
            $champion->last_sale = date('Y-m-d',strtotime($champion->sales[0]->date_out));
        }
    }

    return View::make('champion.predict')->with('champions', $champions);
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Right, one general opinion coming up:

  • Static's, at least the way you're using them, are just globals in drag. Don't do that. If you need both the View and DB class(or at least some tables from it) in the object, of which you posted a method: either Inject, or lazy-load them. A call to a static method is bound to be slower, and, by the looks of things DB::table is sort-of a factory method. Why call it time and time again, if it's already created and returned the desired object once before. You can injected/lazy-loaded them, or just assign them to a property.
  • Anonymous functions (lambda's or closures, whichever name you prefer) aren't my favorite addition to PHP. They feel, to me at least, like a typical PHP afterthought. In essence, they're just syntactic sugar. What they actually do is create an instance of the Closure class. I'm not sure on the internals, but I wouldn't be at all surprised if your code were to create instances of Closure on each iteration of the loop. This isn't that bad in a language like JavaScript, where function objects are very cheap, but in PHP, it'd be far cheaper to define a private method, and pass a callable array to usort. Something along the lines of usort($champion->sales, array($this, 'privateSorterMethod'); will almost certainly be faster, especially since you can declare & assign the array outside of the loop. This gives you more control on when and where the array is constructed: not in the loop, if you don't put it there, that's for sure.
  • Getting data from the database, and returning the view is doing 2 things, and thus a violation of the Single Responsibility Principle. build your $champions array, and return it back to the controller, or whatever called the get_prediction method. The controller is what should link the Model-layer to the view. That's its job: the controller gets the request, decides what the Model-layer needs to do, and sends the output of the model layer back to the view. The model layer should be unaware of the view and vice-versa.
  • Performing that second query inside the loop is, in all likelihood, not the best way forward. Consider using JOIN, to get all data in one go.
  • Maintainability: if I were to be assigned your project some time in the future, I'd have to check the actual DB's, to see what you're getting from the sales table. even if you're getting all fields, specify them.
  • How are you getting your data? As (assoc-)array's? Objects (Stdclass or another object)? Personally, I prefer data models that are directly mapped to the tables. The properties of these data-models correspond to the field names, and they should have a getter and setter for each property. At least, that's what I'd do.

Notes: Calling a static is slower than calling it once, and assigning its return value for re-use. Adding the getTable methods (see below) does mean added overhead (+1 method call), but the overhead will only be noticeable once.
Dropping the anonymous function means better performance, I've made a test-script, and it using a callable array is 50~100% faster. Caching enabled or not, it's the fastest way to go, still.
Fetching rows as data-models with predefined properties is faster than using single stdClass instances, at least: as far as property access is concerned. Basically because the lookup is done using a hashtable, to get the offset of the corresponding zval*. A bit more details here


Just to clarify what I mean by Inject or lazy-load them:

class Foo
{
    private $table = null;
    privte $view = null;//this is a bad idea anyway, but just as an example
    public function __construct(array $tables = array(), View $view = null)
    {//you can pass DB and or view instance to constructor
        $this->tables = $tables;
        $this->view = $view;
    }
    public function setTables(Table $inject)
    {//if you didn't pass to constructor, or need another DB instance for something:
        $this->table[$inject->getName()] = $inject;
    }
    public function setView(View $view)
    {//this is standard DI (dependency Injection)
        $this->view = $view;
    }
    //This is GUARANTEED to return an instance of DB
    //Unless a fatal error occurred.
    protected function getTable($name)
    {
        if (!isset($this->table[$name]))
        {//if table isn't set, load instance on the fly => lazy-loading
            $this->table[$name] = DB::table($name);//get the instance
        }
        return $this->table[$name];
    }
    private function sortCallback($a, $b)
    {
        return strcmp($b->days_since, $a->days_since);
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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