Simple codeigniter refactoring question -- best practice

Question

Hi all,

I have a quick question about refactoring php code. Below are three functions. The first two appear quite similar, and only differ with one if statement. The third combines the first two through use of a flag. Is this the best practice? Here it seems okay to use a flag, but what if we need to add more flags in the future? What is the best practice?

Thanks.

function check_contact_email($email) 
{ 
  $this->db->select('COUNT(login) AS count'); 
  $this->db->from('users'); 
  $this->db->where('email', $email); 
  $query = $this->db->get(); 
  $row = $query->row(); 
  return ($row->count > 0); 
} 

function check_contact_email_id($email) 
{ 
  $this->db->select('COUNT(login) AS count'); 
  $this->db->from('users'); 
  $this->db->where('email', $email); 
  $this->db->where('user_id !=', $_POST['user_id']); 
  $query = $this->db->get(); 
  $row = $query->row(); 
  return ($row->count > 0); 
} 

function check_contact_email($email, $id = FALSE) 
{ 
  $this->db->select('COUNT(login) AS count'); 
  $this->db->from('users'); 
  $this->db->where('email', $email); 
  if ($id) $this->db->where('user_id !=', $_POST['user_id']); 
  $query = $this->db->get(); 
  $row = $query->row(); 
  return ($row->count > 0);  
}

Answer 1

The third function does take the place of the first two. But if you expecting to have more flags, I think you'd want to make a more generic method. What seems to change between the methods is the where clause, so if you make a method with the where clause parameterized, that would handle this situation. Note: I don't know php at all, I'm just basing this off of the look of the code.

Answer 2

Firstly you can reduce this all by using some lesser-known (but documented) ActiveRecord methods like this:

function check_contact_email($email) 
{ 
  $this->db->where('email', $email); 
  return $this->db->count_all_results('users') > 0; 
} 

function check_contact_email_id($email) 
{ 
  $this->db->where('user_id !=', $_POST['user_id']); 
  return $this->check_content_email($email); 
} 

function check_contact_email($email, $id = FALSE) 
{ 
  if ($id) $this->db->where('user_id !=', $_POST['user_id']); 
  return $this->check_content_email($email); 
}

You can reduce this more by passing an array for the flags:

function check_contact_email($params) 
{ 
    if( is_array($params) )
    {
     $this->db->where($params);
    }

    else
    {
     $this->db->where('email', $params);
    } 

    return $this->db->count_all_results('users') > 0; 
}

With that you have one function that can act in various ways:

$this->your_model->check_contact_email($email);

$this->your_model->check_contact_email(array(
    'email' => $email,
    'id !=' => $this->input->post('user_id')
));

$this->your_model->check_contact_email(array(
    'email' => $email,
    'id !=' => $this->input->post('user_id'),
    'otherfield' => $whatever
));

It's not perfect MVC to put that TINY database logic (the !=) in your controller, but its equally bad to put form data directly into your model functions so go with whichever you feel most flexible.