1
\$\begingroup\$

I coded a small user script with PHP. I'm improving the script, learning from my mistakes. I recently learned about object oriented programming and wrote 2 big god classes (user class and message class). That didn't feel right, so read about the MVC model. I wrote a controller class to handle the big god classes while being easy to use in the views (I want to be able to rewrite the big god classes without rewriting the views).

This controller class allows me to streamline my big god classes, which is exactly what I wanted to achieve.

I'm replacing the method user_search() with user_search_info(). It should return a row of user information based on the given user_id or user_id. It should return false if the query fails or returns empty. When I am satisfied, I will delete user_search().

The new method works like I want it to, and I find it very elegant and easy to read. But is it the right approach? I'm sure there's still room for improvement.

This is from the user class file:

  class user{
    // new methods don't care about this anymore so this will get deleted
    public $logs = array();

    // ... irrelevant methods left out ...

    // old method
    public function user_search($db_connection, $user_id=null, $username=null){
      $result = false;
      if($user_id == null && $username == null)
        $this->logs[] = "No information to search on has been found.";
      elseif($db_connection == false)
        $this->logs[] = "Something went wrong with the database.";
      else{
        $this->logs[] = "Searching the user in the database.";
        $query_user_search = $db_connection->prepare('SELECT * FROM users WHERE user_username=:user_username OR user_id=:user_id');
        $query_user_search->bindValue(':user_username', $username, PDO::PARAM_STR);
        $query_user_search->bindValue(':user_id', $user_id, PDO::PARAM_STR);
        $query_user_search->execute();
        if($result = $query_user_search->fetch())
          $this->logs[] = "We found the user.";
        else
          $this->logs[] = "We didn't find the user.";
      }
      return $result;
    }

    // new method
    public function user_search_info($db_con, $user_id=null, $user_username=null){
      $query_user_search_info = $db_con->prepare('SELECT * FROM users WHERE user_username=:user_username OR user_id=:user_id');
      $query_user_search_info->bindValue(':user_id', $user_id, PDO::PARAM_STR);
      $query_user_search_info->bindValue(':user_username', $user_username, PDO::PARAM_STR);
      $query_user_search_info->execute();
      return $query_user_search_info->fetch();
    }
  }

And this is from the controller class file:

public function return_user_search_info($search_user_variable, $search_user_value)
{
  $user_search_info = false;
  if($search_user_variable == "user_id")
    $user_search_info = $this->user->user_search_info($this->db_con, $search_user_value, "");
  elseif($search_user_variable == "user_username")
    $user_search_info = $this->user->user_search_info($this->db_con, "", $search_user_value);
  return $user_search_info;
}
\$\endgroup\$
1
\$\begingroup\$

What you have isn't really a function to search for a user, but a function to get a user, either by id or by name.

So instead of having one function that does two different things, why not have two functions?

public function getUserById($db_con, $id){
    $queryGetUser = $db_con->prepare('SELECT * FROM users WHERE user_id=:id');
    [...]
}

public function getUserByUsername($db_con, $username){
    $queryGetUser = $db_con->prepare('SELECT * FROM users WHERE user_username=:username');
    [...]
}

Now you don't have to pass empty strings to the function (which is really confusing to use), and the functions have a proper name as well.

\$\endgroup\$
  • \$\begingroup\$ I think you are correct. Your solution also makes code easier to read in the controller. I wrote the method when my view pages were directly interacting with the user class. Before I had a controller, I preferred having a slightly larger method over a second large method with 95% the same code. \$\endgroup\$ – Max Jun 25 '16 at 15:34
0
\$\begingroup\$

Thanks to tim's reply, I was able to create an improved version.

  • I split the method in 2 different methods
  • I changed the names of the methods to better describe the purpose of the method
  • Added bonus: code is easier to read and controller doesn't have to pass empty strings to the model.

Updated methods in the user class (as part of the model):

public function get_user_by_id($db_con, $id){
  $query_get_user = $db_con->prepare('SELECT * FROM users WHERE user_id=:id');
  $query_get_user->bindValue('id', $id, PDO::PARAM_STR);
  $query_get_user->execute();
  return $query_get_user->fetch();
}
public function get_user_by_username($db_con, $username){
  $query_get_user = $db_con->prepare('SELECT * FROM users WHERE user_username=:username');
  $query_get_user->bindValue('username', $username, PDO::PARAM_STR);
  $query_get_user->execute();
  return $query_get_user->fetch();
}

Updated method in the controller class:

public function get_user($variable, $value){
  $user = false;
  if($variable == "id")
    $user = $this->user->get_user_by_id($this->db_con, $value);
  elseif($variable == "username")
    $user = $this->user->get_user_by_username($this->db_con, $value);
  return $user;
}
\$\endgroup\$

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.