0
\$\begingroup\$

I am working on a PHP MVC project for my portfolio. Its an web app where you can store contacts (email and phone number). I am having trouble with filtering contacts based on filter form that has 3 options https://ibb.co/8dJbSWd

I wrote code that works fine but I am sure there is better way of doing this. Here is the code that I wrote for solving this problem.

public function filterContacts($user_id, $group, $email, $phone){

    $query = 'SELECT * FROM contacts WHERE user_id = :user_id ';

    //Nothing is selected
    if($group == '0' && $email == '' && $phone == ''){

        $this->db->query($query . ' ORDER BY name');
        $this->db->bind(':user_id', $user_id);
        return $this->db->resultSet();

        //Everything is selected
    } else if ($group !== '0' && $email !== '' && $phone !== ''){

        $query .= 'AND contact_group = :group AND email != "" AND phone_number != ""';

        $this->db->query($query . 'ORDER BY name');
        $this->db->bind(':user_id', $user_id);
        $this->db->bind(':group', $group);

        return $this->db->resultSet();

        //Just group
    } else if ($group !== '0' && $email == '' && $phone == ''){

        $query .= 'AND contact_group = :group ';

        $this->db->query($query . 'ORDER BY name');
        $this->db->bind(':user_id', $user_id);
        $this->db->bind(':group', $group);

        return $this->db->resultSet();

        //Just email
    } else if ($group == '0' && $email !== '' && $phone == ''){

        $query .= 'AND email != "" ';

        $this->db->query($query . 'ORDER BY name');
        $this->db->bind(':user_id', $user_id);


        return $this->db->resultSet();

        //Just phone
    }  else if ($group == '0' && $email == '' && $phone !== ''){

        $query .= 'AND phone_number != "" ';

        $this->db->query($query . 'ORDER BY name');
        $this->db->bind(':user_id', $user_id);

        return $this->db->resultSet();

        //Group and email
    } else if($group !== '0' && $email !== '' && $phone == ''){

        $query .= 'AND contact_group = :group AND email != ""';

        $this->db->query($query . 'ORDER BY name');
        $this->db->bind(':user_id', $user_id);
        $this->db->bind(':group', $group);

        return $this->db->resultSet();

        //Group and phone number
    } else if($group !== '0' && $email == '' && $phone !== ''){
        $query .= 'AND contact_group = :group AND phone_number != ""';

        $this->db->query($query . 'ORDER BY name');
        $this->db->bind(':user_id', $user_id);
        $this->db->bind(':group', $group);

        return $this->db->resultSet();

        //Email and phone number
    } else if($group == '0' && $email !== '' && $phone !== ''){

        $query .= 'AND phone_number != "" AND email != ""';

        $this->db->query($query . 'ORDER BY name');
        $this->db->bind(':user_id', $user_id);

        return $this->db->resultSet();
    }

}

As you can see I used a lot of if statements to make different queries in Contact model which is possible because i have only 3 filtering options but I cannot imagine doing this with 10 options.

So I was wondering is there a better way of solving this?

Here is link to my github repository if you want to see rest of the code https://github.com/Smiley-dev/Phonebook-demo

\$\endgroup\$
0
0
\$\begingroup\$

I must say, I don't find your method to be very scalable because every time you want to add an additional column filter, you will need to add another function argument. Furthermore, what happens when you want to express a different sorting direction or entertain a more sophisticated sorting algorithm with multiole columns? Right now you have four arguments:

public function filterContacts($user_id, $group, $email, $phone) {

This is relatively inflexible.

In the future, if you want to add $status, $mobile, $state, $shoeSize, $bloodType, $birthYear ...you get my point... your method declaration will be threatening to exceed (if not blowing out) the recommended line width limits.

What I recommend is that you design your method now to embrace the possibility of change (so that you won't need to do a major refactor when it is much, much bigger).

public function getContacts($filterBy = [], $orderBy = []) {
    ...
}

Then before your controller calls this function, you determine which qualifying filters and sorting settings are to be passed in and you contruct two strictly formatted arrays which will be consumed by the new model method.

The advantage of setting default values in the method declaration is that you can request a full table dump if you pass no arguments from the controller or you can send $filterBy and omit $orderBy and everything ticks along smoothly with no unnecessary script bloat.

Initially, you may want a simple associative array to port this data, but as your method requirement become more articulate, you may need to develop a multidimensional array structure -- mercifully, I'll avoid dragging you down that rabbit hole for now.

As for the guts of the new method, for utility (until you have a compelling reason to specifically name the result set columns or you need to JOIN tables), just use SELECT * and FROM contacts.

Then implement a battery of checks as you traverse your filtering array on the key (column reference) - value (filtering data value for the column) pairs and insert ? placeholders into the sql statement and build a flat array of parameters to be fed into the execute() call (no need for bind calls).

\$\endgroup\$
3
  • \$\begingroup\$ Ofc it was me. the other answer is so incomparably better and seeing this one accepted is so frustrating that I just cant help it \$\endgroup\$ – Your Common Sense Nov 27 '19 at 9:35
  • \$\begingroup\$ There are other ways to improve SE site content than downvoting. You know I'll listen to what you have to say. If you are just having a classic, venomous YCS moment, I understand -- I've seen it many times before. Obviously I didn't spend so much of my volunteer time writing up this answer because I wanted to poison the knowledge pool. I can at least appreciate your courage to comment with your downvote -- so few do. I will absorb your unconstructive negativity and harbor no vengeful thoughts. I respect your best qualities and pity your foibles. \$\endgroup\$ – mickmackusa Nov 27 '19 at 9:40
  • \$\begingroup\$ Thank you a lot. I think I finally wrote a clean and flexible solution \$\endgroup\$ – Smiley-dev Nov 27 '19 at 14:04
3
\$\begingroup\$

The entire long conditional is definitely worth to be decomposed and consolidated.

The main mechanics of optimization is based on 3 aspects:

  • determining a common logic/behavior that needs to be unified. In your case such a common fragment is:

    $this->db->query($query . 'ORDER BY name');
    $this->db->bind(':user_id', $user_id);
    return $this->db->resultSet();
    
  • considering only populated fields

  • collecting bindings for particular fields

The final optimized function becomes as below:

public function filterContacts($user_id, $group, $email, $phone){

    $query = 'SELECT * FROM contacts WHERE user_id = :user_id';
    $bindings = [':user_id':=> $user_id];

    if ($group !== '0') {
        $query .= ' AND contact_group = :group';
        $bindings[':group'] = $group;
    }    
    if ($email !== '') {
        $query .= ' AND email != ""';
    }
    if ($phone !== '') {
        $query .= ' AND phone_number != ""';
    }

    $this->db->query($query . ' ORDER BY name');
    foreach ($bindings as $k => $v) {
        $this->db->bind($k, $v);
    }

    return $this->db->resultSet();
}

In case if your DB clent $this->db allows to pass an array of bindings you can just replace the loop with $this->db->bind($bindings); (check your DB client interface)

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you a lot. I think I finally wrote a clean and flexible solution \$\endgroup\$ – Smiley-dev Nov 27 '19 at 14:04
0
\$\begingroup\$

At the end I wrote something like this

Controler method

public function filter(){

        if ($_SERVER['REQUEST_METHOD'] == 'POST'){

            $filters = $_POST;

            $contacts = $this->contactModel->getContacts($_SESSION['user_id'], $filters);

            $data = [
                'contacts' => $contacts
            ];

            $this->view('contacts/table', $data);
        }

    }

Model method

public function getContacts($user_id, $filterBy = []){


        $query = 'SELECT * FROM contacts WHERE user_id = :user_id ';
        $bindings = [':user_id' => $user_id];

        if(!empty($filterBy)){

            if($filterBy['group'] !== '0'){
                $query .= 'AND contact_group = :group ';
                $bindings[':group'] = $filterBy['group'];
            }
            if($filterBy['email'] !== 'false'){
                $query .= 'AND email != "" ';
            }
            if($filterBy['phone'] !== 'false'){
                $query .= 'AND phone_number != "" ';
            }

            if($filterBy['search'] !== ''){
                $query .= 'AND name LIKE :search ';
                $bindings[':search'] = '%' . $filterBy['search'] . '%';
            }
        }

        $this->db->query($query);
        foreach ($bindings as $key => $value){
            $this->db->bind($key, $value);
        }

        return $this->db->resultSet();

    }

It works fine and it seems good enough for me. Thanks a lot for your help

\$\endgroup\$
1
  • \$\begingroup\$ if(!empty($filterBy)){ is superfluous. you can remove this condition and it wouldn't change anything. \$\endgroup\$ – Your Common Sense Nov 28 '19 at 7:12

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.