6
\$\begingroup\$

I've built a Search Engine class for a website that permits to search companies in base of their locations and their categories (companies pay a plan to be found in the places that they will).

In the constructor you can pass an index array cointaining the parameters or you can set them later with setters. All parameters are optional.

Basically calling getQueryBuilder() method you obtain a Laravel's Query Builder instance and can treat it as you want (for example use Pagination etc.)

Note: I did not designed the structure of the database, I know that it's not the best (comma separated values etc.) but I cannot touch it.

I would like to know if the design of the class is good or not, and possibily learn how to build it properly.

class SearchEngine
{

    private $region = null;
    private $province = null;
    private $zipcode = null;
    private $municipality = null;
    private $category = null;
    private $subcategory = null;
    private $b2b = false;

    /*
     *
     * Possible $params keys:
     * 'region' --> The Region id
     * 'province' --> The Province id
     * 'zipcode' --> The Zipcode
     * 'municipality' --> The Municipality id
     * 'category' --> The category code
     * 'subcategory' --> The subcategory code
     * 'b2b' --> Find only companies with b2b market type
     * 
     *  All values MUST be integer or null except 'b2b', that MUST be a boolean
     *
     * @author xxxxxxx
     * @param array $params
     *
     *
     */

    public function __construct($params = []){
        if(!is_array($params)){
            throw new \InvalidArgumentException("Params MUST be an array");
        }

        foreach($params as $key => $value){
            if($key == 'b2b'){
                $this->b2b = boolval($value);
            }
            else {
                $this->{$key} = intval($value);
            }
        }

    }

    /**
     * @return null or int
     */
    public function getRegion()
    {
        return $this->region;
    }

    /**
     * @param $region
     */
    public function setRegion($region)
    {
        $this->region = (is_null($region) ? null : intval($region));
    }

    /**
     * @return null or int
     */
    public function getProvince()
    {
        return $this->province;
    }

    /**
     * @param $province
     */
    public function setProvince($province)
    {
        $this->province = (is_null($province) ? null : intval($province));
    }

    /**
     * @return null or int
     */
    public function getZipcode()
    {
        return $this->zipcode;
    }

    /**
     * @param $zipcode
     */
    public function setZipcode($zipcode)
    {
        $this->zipcode = (is_null($zipcode) ? null : intval($zipcode));
    }

    /**
     * @return null or int
     */
    public function getMunicipality()
    {
        return $this->municipality;
    }

    /**
     * @param $municipality
     */
    public function setMunicipality($municipality)
    {
        $this->municipality = (is_null($municipality) ? null : intval($municipality));
    }

    /**
     * @return null or int
     */
    public function getCategory()
    {
        return $this->category;
    }

    /**
     * @param $category
     */
    public function setCategory($category)
    {
        $this->category = (is_null($category) ? null : intval($category));
    }

    /**
     * @return null or int
     */
    public function getSubcategory()
    {
        return $this->subcategory;
    }

    /**
     * @param $subcategory
     */
    public function setSubcategory($subcategory)
    {
        $this->subcategory = (is_null($subcategory) ? null : intval($subcategory));
    }

    /**
     * @return boolean
     */
    public function isB2b()
    {
        return $this->b2b;
    }

    /**
     * @param boolean $b2b
     */
    public function setB2b($b2b)
    {
        $this->b2b = boolval($b2b);
    }

    public function resetParams() {
        foreach ($this as $key => $value) {
            $this->$key = null;
        }

        $this->b2b = false;
    }

    /*
     * Returns query builder instance with given params
     *
     * @return Illuminate\Database\Query\Builder
     */
    public function getQueryBuilder(){
        return Company::whereRaw($this->generateQuery());
    }

    /*PRIVATE FUNCTIONS*/

    /*
    * Generates the query to pass to the query builder
    *
    * @return string
    */
    private function generateQuery(){
        $sql = "id IN (
                SELECT bp.company_id FROM
                (
                    buy_plansets bp
                    INNER JOIN buy_geocategories bg
                    ON bg.planset_id=bp.id
                )

                WHERE
                bp.deleted_at IS NULL AND
                bg.deleted_at IS NULL AND
                bp.subscribe_status IN (".Buy_planset::get_positive_status_csv().") AND
                bp.subscribe_date_on<=curdate() AND(
                    bp.subscribe_date_off IS NULL OR
                    bp.subscribe_date_off>=curdate()
                )
        ";

        $and_where = [];

        if(!is_null($this->municipality)) {
            $and_where[] = "(
                            bg.municipality_csv regexp " . $this->regexWrap($this->municipality) . "
                            OR (bg.region_all=1 AND bg.region_id=".$this->getRegionByMunicipality().")
                            OR (bg.province_all=1 AND bg.province_id=".$this->getProvinceByMunicipality().")
                            OR bg.zip_csv regexp " . $this->getFormattedZipcodesByMunicipality() . "
                            )";

        }elseif(!is_null($this->zipcode)){
            $and_where[] = "(
                            bg.zip_csv regexp ".$this->regexWrap($this->zipcode)."
                            OR (bg.region_all=1 AND bg.region_id=".$this->getRegionByZipcode().")
                            OR (bg.province_all=1 AND bg.province_id=".$this->getProvinceByZipcode().")
                            )";

        }elseif(!is_null($this->province)){
            $and_where[] = "(
                            bg.province_id=".$this->province."
                            OR (bg.region_all=1 AND bg.region_id=".$this->getRegionByProvince().")
                            )";
        }elseif(!is_null($this->region)){
            $and_where[] = "bg.region_id=" . $this->region;
        };

        if(!is_null($this->subcategory)){
            $and_where[] = "(
                            bg.subcategory_code_csv regexp ".$this->regexWrap($this->subcategory)."
                            )";
        }elseif(!is_null($this->category)){
            $and_where[] = "(
                            bg.subcategory_code_csv regexp ".$this->getFormattedSubcategories()."
                            )";
        }

        $sql .= " AND " . ($and_where ? implode(" AND ", $and_where): "1=1");
        $sql .= ") " . ($this->b2b ? "AND market_type=".Company::MARKET_BB : '');

        return $sql;
    }

    /*
     * Wraps a value in regex to search in csv values
     *
     * @return string
     */
    private function regexWrap($value, $brackets = false){
        $reg_start = "'[[:<:]]";
        $reg_end = "[[:>:]]'";

        if($brackets){
            $exp = $reg_start . "(" . $value . ")" . $reg_end;
        }
        else{
            $exp = $reg_start . $value . $reg_end;
        };

        return $exp;

    }

    /*
    * Returns formatted zipcodes ids to be used in mysql regex
    *
    * @return string
    */
    private function getFormattedZipcodesByMunicipality(){
        $zip_ids = DB::select("SELECT group_concat(z.id separator '|') AS zips
                              FROM (
                              municipalities m inner join
                              municipality_zipcode mz
                              on m.id=mz.municipality_id
                              inner join zipcodes z
                              on z.id=mz.zipcode_id
                              )
                              WHERE m.id=?", [$this->municipality])[0]->zips;

        return $this->regexWrap($zip_ids, true);
    }

    private function getRegionByMunicipality(){
        return Municipality::findOrFail($this->municipality)->province->region->id;
    }

    private function getProvinceByMunicipality(){
        return Municipality::findOrFail($this->municipality)->province->id;
    }

    private function getRegionByZipcode(){
        return Zipcode::findOrFail($this->zipcode)->province->region->id;
    }

    private function getProvinceByZipcode(){
        return Zipcode::findOrFail($this->zipcode)->province->id;
    }

    private function getRegionByProvince(){
        return Province::findOrFail($this->province)->region->id;
    }

    /*
    * Returns formatted subcateogories codes to be used in mysql regex
    *
    * @return string
    */
    private function getFormattedSubcategories(){
        $cat_codes = Category::where("parent_code", $this->category)
                                ->selectRaw("group_concat(`code` separator '|') as subcat")
                                ->value("subcat");

        return $this->regexWrap($cat_codes, true);
    }


}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some good answers. \$\endgroup\$
    – Phrancis
    May 11, 2016 at 11:04

1 Answer 1

6
\$\begingroup\$

I really like the structure of your code. That said, I almost never suggest to give class variables default values. That is what the constructor is for. For that matter, you don't need to specify that the object's equal null as they already default to that when created.

In regards to optional inputs inside of the array, make sure you validate your input. While you are using inval and boolval, there's no guarantee that the input is what you need to properly parse the value. If you end up passing a string to intval, you're likely not to get the result intended (either an exception or 0 depending on how you handle it). Same goes for boolval. It should be receiving true, false, 0, or 1.

Moving further in, I understand the need for a raw query in the generateQuery() function, however, it might be smarter to use straight up PDO here, versus the Query Build which calls PDO. My reasoning here is that you are using a raw query statement with an object passed straight into the query. The Laravel Query Builder only protects you from SQL injection if you pass the objects as binds, which can't be done in rawQuery. For that reason, rewriting it as a PDO::PreparedStatement might be smarter as you can then pass a bindings array instead of putting the object right into the SQL.

While it is closed as off-topic, this SO Question does give some good resources when looking to implement a PDO connection.

\$\endgroup\$
1
  • \$\begingroup\$ Glad I could help! :) \$\endgroup\$
    – Trojan404
    Jul 15, 2016 at 14:28

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.