4
\$\begingroup\$

I have this function that returns me a list of users with their roles and groups. As you can see, this is how I fetch and create list of objects. I'm wondering whether this is a good approach and what parts should be improved. I'm not that experienced with PHP so I would appreciate code samples.

I'm also wondering how good an approach it is to first get all users and then make another prepared statement to get user roles and groups. That would mean I will have big number of database calls, so I think it's a bad idea.

    $stmt = $mysqli->prepare("SELECT u.id, u.firstName, u.lastName, u.email, 
        u.phoneNumber, u.address, u.birthDate, ur.roleName, cg.id, cg.name FROM users as u 
        LEFT OUTER JOIN user_role as ur ON u.id = ur.userId 
        LEFT OUTER JOIN user_group as ug on ug.userId = u.id 
        LEFT OUTER JOIN control_group as cg on cg.id = ug.groupId WHERE u.id != ?");
    $stmt->bind_param("i", $_SESSION["id"]);
    $stmt->execute();
    $stmt->bind_result($id, $firstName, $lastName, $email, $phoneNumber, 
        $address, $birthDate, $roleName, $groupId, $groupName);
    $users = array();

    while ($stmt->fetch()) {
        if (empty($users[$id])) {
            $users[$id] = array(
                'id' => $id,
                'firstName' => $firstName,
                'lastName' => $lastName,
                'email' => $email,
                'phoneNumber' => $phoneNumber,
                'address' => $address,
                'birthDate' => $birthDate,
                'roles' => array(),
                'groups' => array()
            );
        }
        if ($roleName) {
            $found = false;
            foreach ($users[$id]['roles'] as $role) {
                if($role['roleName'] == $roleName){
                    $found = true;
                    break;
                }
            }
            if($found == false)
                $users[$id]['roles'][] = array(
                    'roleName' => $roleName
                );
         }

        if ($groupId) {
            $found = false;
            foreach ($users[$id]['groups'] as $group) {
                if($group['groupName'] == $groupName){
                    $found = true;
                    break;
                }
            }
            if($found == false)
                $users[$id]['groups'][] = array(
                    'groupName' => $groupName
                );
         }
    }

    $stmt->close();
    $mysqli->close();
    echo json_encode($users);

This is the response I get, only thing that I want improve is item index, as you can see in my example as index I have item id, I would like to get correct index starting from 0.

\$\endgroup\$
  • \$\begingroup\$ Is the code you currently have working like you want to? We can review your code if it works, but feature requests are considered off-topic. \$\endgroup\$ – Mast Feb 26 '17 at 18:17
  • 1
    \$\begingroup\$ It is working like i want, i just want to know is there a better way to generate this object, and is there something else that i could improve. \$\endgroup\$ – Super Mario's Yoshi Feb 26 '17 at 18:18
1
\$\begingroup\$

When you say you want "correct index starting from 0", does this mean that you want the first dimension in your data structure to be 0 to n-1 (where n is number of user objects returned)?

If that is the case, you really should be building a numerically-indexed array of user objects, not an object/associative array with the user id as the first dimensional key.

Some thoughts on your code:

  • I would strongly consider moving away from camelCasing the names of your database entities (table, columns, etc.). This can save you from problem around the fact that, in most cases with MySQL, these database entities are treated without consideration for case (like in files on your system and such). Using snake_case is generally the preferred approach for most relational database systems to avoid unexpected problems arising from such naming.
  • Be consistent on upper-casing all query parts that are not database entities - AS, ON, etc. in addition to the obvious ones like SELECT, FROM, etc. Right now you are mixing usage for on.
  • Take as much vertical space as you need when writing your queries. You get no bonus points for trying to keep it on as few lines as possible. Err on the side of making everything in your code more readable.
  • I would consider aliasing the return field names in your query so you can move away from binding query results with variables that are available in the main scope of this script. To me you begin to lose the concept of working with a row of data in your current approach.
  • You might consider using fetch_object() in combination with the aliasing noted above, to give you a nice, readable way to work with each row in the result set. This will also map null values in result set to true null values on the resulting row object.
  • You should use ORDER BY clauses whenever you have cases where you are going to need to map flat rows from a result set into a hierarchical data structure. This allows you to simply look for changes in column values when iterating the structure to trigger the need to create a new child data structures.
  • If you truly work with objects when reading data into your final structure and use an ORDER BY clause, you should be able to simplify your result row to data structure mapping logic.
  • Why be redundant in your response data with having labels roleName and groupName when they are already nested in roles and groups arrays?
  • Why retrieve group id at all if you are not going to use it in the resulting response structure?

Putting this all together you might have code more like this:

$query = "
SELECT
    u.id AS id,
    u.first_name AS first_name,
    u.last_name AS last_name ,
    u.email As email,
    u.phone_number AS phone_number, 
    u.address AS address,
    u.birth_date AS birth_date,
    ur.roleName AS role_name,
    cg.name AS group_name
FROM users AS u 
LEFT OUTER JOIN user_role AS ur ON u.id = ur.user_id 
LEFT OUTER JOIN user_group AS ug ON ug.user_id = u.id 
LEFT OUTER JOIN control_group AS cg ON cg.id = ug.group_id
WHERE u.id != ?
ORDER BY id ASC, role_name ASC, group_name ASC
";
$stmt = $mysli->prepare($query);
$stmt->bind_param("i", $_SESSION["id"]);
$stmt->execute();

// and then fetch rows
$users = array();
$user = new stdClass();
$user->id = 0;
while ($row = $stmt->fetch_object()) {
    // build new user if needed
    if((int)$row->id !== $user->id) {
        // Break existing reference between previous $user and $users.
        // Data in $users will remain after this dereferencing
        unset($user);
        $user = new stdClass();
        $user->id = (int)$row->id;
        $user->firstName = $row->first_name;
        $user->lastName = $row->last_name;
        $user->email = $row->email;
        $user->phoneNumber = $row->phone_number;
        $user->address = $row->address;
        $user->birthDate = $row->birth_date;
        $user->roles = array();
        $user->groups = array();
        // Assign this new user object by reference to $users.
        // This allows you to simply work with $user here in loop
        // as opposed to $users[$index] which requires you to manually
        // track index values.
        $users[] =& $user;

        // since this is a new user object, we need to reset
        // role and group objects so we can detect changes in these columns.
        unset($role);
        $role = new stdClass();
        $role->name = null;
        unset($group);
        $group = new stdClass();
        $group->name = null;
    }
    // build new role if needed
    if($row->role_name !== $role->name) {
        // dereference role
        unset($role);
        // create new role for this user
        $role = new stdClass();
        $contact->name = $row->role_name;
        $user->roles[] =& $role;
    }
    // build new group if needed
    if($row->group_name !== $group->name) {
        // dereference group
        unset($group);
        // create new group for this user
        $group = new stdClass();
        $group->name = $row->group_name;
        $user->groups[] =& $group;
    }
}
unset($user, $role, $group);

$stmt->close();
$mysqli->close();
echo json_encode($users);

Note that this is simple example and doesn't have proper error handling around statement preparation and execution. You should make sure that you just don't assume these things work. Make sure you understand all possible results and/or exceptions that can occur from a function/method call and handle those outcomes accordingly.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your awesome suggestions, i will try to implement everything what you suggested. Also can you explain me how important is to change camel case to snake case table attributes ? \$\endgroup\$ – Super Mario's Yoshi Mar 1 '17 at 22:38
  • \$\begingroup\$ @SuperMario'sYoshi Some additional reading on the topic you are asking about - stackoverflow.com/questions/14317784/… and from MySQL manual dev.mysql.com/doc/refman/5.6/en/… \$\endgroup\$ – Mike Brant Mar 1 '17 at 22:46
  • \$\begingroup\$ @SuperMario'sYoshi I guess I didn't really answer your question. I would not think it something that is critical assuming you are not going to be changing out database platforms anytime in the future. I would consider it more of a best practice. If you already have a mature application, you may not find much value in making such a schema change at this point, whereas if this is a brand new app and not much is tied to these tables yet, it may make sense to change. \$\endgroup\$ – Mike Brant Mar 1 '17 at 22:55
  • \$\begingroup\$ Mark, @Super I will allow myself a little review on review :) Some of statements in this post are rather irrelevant. For example, using objects won't give you any benefit: with arrays the resulting JSON will be exactly the same. Ordering results also won't make any benefit in this particular case. The statement on aliasing field names is factually incorrect, as binding results is required by the API, not the programmer's will. This code is not very optimal as it selects a lot of duplicated data from database. And, in the end, having more code than before doesn't seem very refactoring to me. \$\endgroup\$ – Your Common Sense Mar 2 '17 at 4:36
2
\$\begingroup\$

For the exact case you posted here, I would suggest to use group_concat() in the query. It will dramatically reduce the amount of code required, as well as the amount of data delivered from database.

Besides, I see a very little use for the prepared statement here, so the amount of code could be dramatically reduced on this account as well.

$sql = "SELECT u.id, u.firstName, u.lastName, u.email, u.phoneNumber, 
    u.address, u.birthDate,group_concat(ur.roleName), roles group_concat(cg.name) groups 
    FROM users as u 
    LEFT OUTER JOIN user_role as ur ON u.id = ur.userId 
    LEFT OUTER JOIN user_group as ug on ug.userId = u.id 
    LEFT OUTER JOIN control_group as cg on cg.id = ug.groupId 
    GROUP BY u.id";
$res = $mysqli->query($sql);
$users = array();
while ($row = $res->fetch_assoc()) {
    if ($row['id'] == $_SESSION["id"]) {
        continue;
    }
    $row['groups'] = explode(",", $row['groups']);
    $row['roles'] = explode(",", $row['roles']);
    $users[] = $row;
}

This is the response example which I consider most usable and clear.
But note it would take no trouble to make it exactly as in your example above, if you wish.

On a second thought, thinking one move ahead, I suppose you may wish to make groups and roles some sort of interactive - allowing clicks or other interactions with them. And for this purpose you will need group and roled IDs inevitably.

Therefore, to make this code robust and usable in the real life app, you have to supply group and role id as well. For this purpose I can't help it but use PDO, as its helper methods do A LOT of your job. For example, PDO::FETCH_KEY_PAIR gives you nice key-value pairs right out of SQL query:

$groups = $pdo->query("SELECT id, name FROM control_group")->fetchAll(PDO::FETCH_KEY_PAIR);
$roles  = $pdo->query("SELECT id, roleName FROM user_role")->fetchAll(PDO::FETCH_KEY_PAIR);

$sql = "SELECT u.id, u.firstName, u.lastName, u.email, u.phoneNumber, 
    u.address, u.birthDate,group_concat(ur.roleName), roles group_concat(cg.name) groups 
    FROM users as u 
    LEFT OUTER JOIN user_role as ur ON u.id = ur.userId 
    LEFT OUTER JOIN user_group as ug on ug.userId = u.id 
    LEFT OUTER JOIN control_group as cg on cg.id = ug.groupId 
    GROUP BY u.id";
$res = $pdo->query($sql);
$users = array();
while ($row = $res->fetch(PDO::FETCH_ASSOC)) {
    if ($row['id'] == $_SESSION["id"]) {
        continue;
    }

    $user_groups = array();
    foreach(explode(",", $row['groups'] as $id) {
        $user_groups[$id] = $groups[$id]
    }
    $row['groups'] = $user_groups;

    $user_roles = array();
    foreach(explode(",", $row['roles']) as $id) {
        $user_roles[$id] = $roles[$id]
    }

    $row['roles'] = $user_roles;
    $users[] = $row;
}

This is a response example.

As you can see, here we're first selecting all the group and role names, while in the main query selecting only ids. Then in the short loop finally creating groups and roles arrays.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your great suggestions, but at my project im using mysqli with mysqli orm library, anyway i will probably try to switch to PDO, in terms of sqli can you give me some feedback, also can you show me how response should look with your suggestions ? \$\endgroup\$ – Super Mario's Yoshi Feb 28 '17 at 16:28
  • \$\begingroup\$ I updated the answer with mysqli. As of the result, I can't give you an example as I don't have your data. Basically you have plain arrays with group names, without repeated groupName stuff: just "groups":["Suicide Squad","Justice League"] \$\endgroup\$ – Your Common Sense Feb 28 '17 at 16:34
  • \$\begingroup\$ Got it, thank you very much on your well explained solutions. \$\endgroup\$ – Super Mario's Yoshi Feb 28 '17 at 16:36
  • \$\begingroup\$ GROUP_CONCAT() may be problematic, as you would lose visibility into the different combinations of user_role and user_group relationships for an individual user, as you would be dropping null values from the results (something that may need to be considered with these outer joins). Also be careful using fetch_all() as this could lead to more memory usage than is desired. \$\endgroup\$ – Mike Brant Mar 1 '17 at 21:39
  • \$\begingroup\$ @MikeBrant I agree with your sentiments in general, but for this particular case they are irrelevant. The OP is explicitly dropping out null values in their code and fetch_all is the very goal of their code as well. \$\endgroup\$ – Your Common Sense Mar 2 '17 at 4:57

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.