Refactor Controller based on many get params

It might not be applicable for your case - but I had to do something similar recently and just decided to return a superset of results to the client and then filter it 'live' using JS. From the end users perspective it was actually faster even though I was sending them more raw data in the first instance - from then on it was pretty instant to do whatever sub-selections/filters they wanted. Anyway - it's another way of doing it which might (or might not) be of use :-)

Hi @ohffs , you made me think twice on how should I implement this filter system. Because in my case I have a form with a lot of filters and is ajax based, on each change on the form the results must be reloaded.

Here is a screen of my application:

The green block are the results.

But if I select from database 2k - 3k of properties to filter 'live' with js in not a bad idea?

Also my back-end way is compatible with an API. So in future if I wanna make some queries to get the proper results.

The suggestion @ohffs makes is exactly how I've done filtering in the past. You'll probably be surprised but obviously you'll need to test it with your data. Just be careful what you return to the browser and keep in mind a small amount of extra information per result might be a lot of information when you are dealing with the full result set. Keep your HTML/JSON that describes a single result as clean as possible.

If you feel uncomfortable or run into performance issues another approach might be to cut down the data with a simple user selection. For example it looks like you are dealing with property and you have some properties for rent and some to buy. It's unlikely, in my opinion, someone looking to buy a property would be interested in rental property and vice versa. So ask that question first and send people to a page accordingly with your result set already reduced ...

www.awesome-property.com/for-rent

www.awesome-property.com/for-sale

This is also good practice for search ranking.

There are loads of tools out there that would do client side filtering for you as well. I've used jQuery Isotope on a number of occasions and although it has a small license fee it's so easy to use.

http://isotope.metafizzy.co/

As I said - it's just another way which may or may not be applicable in your case :-) You could test and see how long it takes to return gzipped 2-3k JSON records vs. doing a new request-per-filter and see if it was any use. You're basically off-loading queries to the client - in my case that made the code and server load much better, it might not for you :-) If you have, say, 100 simultaneous users then every time they pick a filter it's another query to your server & db doing it the 'back end' way so you can end up performing 100's of queries all the time - if you do it the 'superset' way it's just one query per user then you don't care ;-)

But I can't say if that works for you - maybe have a play around and see how it feels?

@richwilliamson thank you for your approach.

This is how my website looks like.

I have the form on the top, at any change I make a ajax request and update the results. I also have a pagination, for demo is set to 2 but on production will be to 10. Also I want to have a URL with params, so if you access the

http://example.com/properties?type=apartment&city=new_work-1&area=obcini-1&street=1_decembrie_1918-1&rooms=3&current_floor=8&thermal_power_station=1&pvc_windows=1&insulation=1&parquet=1&hone=0&faience=0&renovated=1&metal_door=0

to get the proper results. I think this can be made with js too. For example if I get all results and based on the URL to filter.

@ohffs I will try both variants, but I think for a API the current version is OK.

Now the main purpose of the question was refactoring :D , I updated with my last version of code, please tell me how can I reduce the code to be more dry and clean? I know that is a lot of code and may be hard to read but I am here to answer. Should I create a helper? A new class?

Ok - depends how cruel you want us to be ;-p

Outside of using the (somewhat) obscure PHP 'template' style (eg, if(thing): ... endif) - it's hard to say as a lot of it is exploding/whatever'ing data that we can't see. But for one :

$city_id            = key_exists('city', $filters) ? explode('-', $filters['city'])[1] : null;
$area_id            = key_exists('area', $filters) ? explode('-', $filters['area'])[1] : null;
$street_id          = key_exists('street', $filters) ? explode('-', $filters['street'])[1] : null;
$transaction        = key_exists('transaction', $filters) ? $filters['transaction'] : null;
...
if(array_key_exists ($characteristic->name, $filters)):

Might be nicer to have :

$this->filters = Input::all();
$city_id = $this->getUntokened('city');  // untokened is a horrible name, but there we go
$area_id = $this->getUntokened('area');
$street_id = $this->getUntokened('street');
$transaction = $this->getFilter('transaction');
if($this->hasFilter($characteristic->name)):
...
protected function hasFilter($key)
{
    return array_key_exists($key, $this->filters);
}

protected function getFilter($key)
{
    return $this->hasFilter($key) ? $this->filters[$key] : null;
}

protected function getUntokened($key)
{
    $value = $this->getFilter($key);
    if (!$value) {
        return null;
    }
    return explode('-', $value)[1];
}

There are bits of code near the bottom like :

elseif($area_id && !$city_id):
                return "Select a city to get a zone!";
endif;

Could those be right at the top so you return 'quick' from the function if you have an invalid condition? Other than that it gets a bit trickier to say. For instance :

$values  = explode(',', $filters[$characteristic->name]);
$id     = $characteristic->id;
foreach($values as $value):
  if(count($values) > 1):
    ....

... Is quite hard to figure out without seeing the potential data - 'something with commas, or something' is all I can make out ;-)

Just another vague anecdotal thing. When there is a block of code like :

 foreach($amenities as $amenity):
                if(array_key_exists ($amenity->name, $filters)):
                    $status  = $filters[$amenity->name];
                    $id     = $amenity->id;
                    $properties = $properties->whereHas('property_amenities', function ($query) use($status, $id) {
                        $query->where('amenity_id', $id)
                            ->where('status', $status);
                    });
                endif;
endforeach;

I try and think 'what is that block trying to do?' and see if I can extract it to something meaningful. So in this case, off the top of my head, you might have :

$properties = $this->addMatchingAmenities($amenities, $properties);
...
protected function addMatchingAmenities($amenities, $properties)
{
   ... blah
   return $properties;
}

And in turn you look at code blocks inside addMatchingAmenties and see if you can extract blocks into friendly named helpers. Even if it doesn't make the code shorter/faster/whatever - next time you or someone else is reading it you at least get the gist of what it's trying to do without having to read through lots of if/else/foreach/explode statements right away :-)

Thank you @ohffs I followed your indications and my code now looks better. I pasted my code on pastebin because is too long to post it here.

http://pastebin.com/1mY2KLGY

applyMatchingCharacteristics() is still ambiguous I think. That explode from getMultipleValueFilter() was to process multiple values from URL.

I.E. http://....../?type=apartment&rooms=3,4&current_floor=8,5

So applyMatchingCharacteristics have another nested foreach-if-foreach-if and I think I will broke this method in 2 small.

And yes, I wanna be cruel with the code ha ha :D

Also the protected methods should not be move in other file to be more organised?

Once you've refactored out to the protected helpers then you can see if you want to make it into a separate class (I guess in this case you will as there will probably be quite a bit of code).

Another example refactor you could do - the two code sections :

if($type):
            $property_type      = PropertyType::where('name', $type)->get()->first();
            $characteristics    = DefaultCharacteristic::where('property_type_id', $property_type->id)->get();
            $amenities          = DefaultAmenity::where('property_type_id', $property_type->id)->get();
endif;

// ... two blocks of code

if(isset($property_type)):
            $properties = $properties->whereHas('property_type', function ($query) use($type) {
                $query->where('name', $type);
            });
            $properties = $this->applyMatchingCharacteristics($characteristics, $properties);
            $properties = $this->applyMatchingAmenities($amenities, $properties);
endif;

It looks like that 2nd block of code could be inside the first? And then maybe extract from there?