1
\$\begingroup\$

Well, I am writing a web service using PHP to return data from a DB in JSON format. I need to query 3 tables, which have a one-to-many relationship, like shown below:

Contact Group ---(has)---> Contact ---(has)---> Phone number

Here I want to get an array of each group, in each group, get an array of all contacts and in each contact, get an array of all phone numbers. Because I am new to PHP, this might not be following best practices for PHP. The code I wrote is below:

$sql = "SELECT r.contact_id, contact_name, group_id, group_name, phone_id, phone_number FROM contact r LEFT JOIN contact_group s ON r.contact_group = s.group_id LEFT JOIN phone_numbers t ON r.contact_id = t.contact_id ORDER BY group_id, contact_id, phone_id";
$query = mysqli_query($conn,$sql);
$data = array();

while ($row = mysqli_fetch_array($query)) {
    $data[$row["group_id"]]["group_id"] = $row["group_id"];
    $data[$row["group_id"]]["group_name"] = $row["group_name"];

    $data[$row["group_id"]]["contacts"][$row["contact_id"]]["id"] = $row["contact_id"];
    $data[$row["group_id"]]["contacts"][$row["contact_id"]]["name"] = $row["contact_name"];

    $data[$row["group_id"]]["contacts"][$row["contact_id"]]["phone_number"][$row["phone_id"]]["id"] = $row["phone_id"];
    $data[$row["group_id"]]["contacts"][$row["contact_id"]]["phone_number"][$row["phone_id"]]["number"] = $row["phone_number"];
}

I think my code is very ugly and seems difficult to read. Could you help me review my code and refactor it to make it more beautiful or introduce new ways to solve that problem?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

First, let's start with the way your query is formatted. I would strongly recommend you get in the habit of breaking your query across lines when they get long like this. I oftentimes use heredoc/nowdoc synteax for this.

For example:

$sql = <<<EOT

SELECT
  r.contact_id,
  contact_name,
  group_id,
  group_name,
  phone_id,
  phone_number
FROM contact r
LEFT JOIN contact_group s
  ON r.contact_group = s.group_id
LEFT JOIN phone_numbers t
  ON r.contact_id = t.contact_id
ORDER BY group_id, contact_id, phone_id

EOT;

This gives you a nice, clean visual presentation for your query in your code.

I think your query as written is a little odd, in that you specify the table for contact_id but not for any of the other columns you are selecting. Some of your column names may be ambiguous across tables, so I try to make it a practice when writing join queries to specify the table names for ALL columns to remove ambiguity for the reader.

I would also suggest aliasing the columns. If you want to refer to contact_id in your PHP code like you are doing, then you MUST alias r.contact_id to this like r.contact_id AS contact_id.

Finally, you should consider meaningful table aliases if using them (I actually typically do not use them except for sub-selects that need to be aliased). What do r, s, and t mean? Why not c, cg, pn or similar?

That could leave you with a query like:

$sql = <<<EOT

SELECT
  contact.contact_id AS contact_id,
  contact.contact_name AS contact_name,
  contact_group.group_id AS group_id,
  contact_group.group_name AS group_name,
  phone_numbers.phone_id AS,
  phone_numbers.phone_number AS phone_number
FROM contact
LEFT JOIN contact_group
  ON contact.contact_group = contact_group.group_id
LEFT JOIN phone_numbers t
  ON contact_group.contact_id = phone_numbers.contact_id
ORDER BY group_id, contact_id, phone_id

EOT;

This is a little more verbose, and might take you a minute or two of extra typing when initially writing the query, but it has the effect of removing any ambiguity for what your query is doing for the reader.


Outside of format, your query seems odd in that it doesn't seem to match the relations you have noted. You said the relationships are:

Contact Group ---(has)---> Contact ---(has)---> Phone number

Yet your joins would seem to suggest relations of:

Contact ---(has)---> Contact Group ---(has)---> Phone number

Can a contact exist that does not belong to a contact_group? Your query suggests this is the case (due to LEFT JOIN), which would beg the question, how are you handling the nulls that could be returned for contact_group fields in such a case?


You are taking a very standard approach to populating a multi-dimensional array from a results set that represents one-to-many relationships.

I do question however if this is an appropriate approach given the fact that you are using LEFT JOINs here. As noted above, you could have null values for the contract group information and the phone number information. You don't seem to be considering this at all.

This is especially critical given the fact that the contact group ID is the very first dimension in your array. Are you really intending to group on null in this first dimension (technically such values would actually be assigned to an empty string array key)?


Since JSON can only represent two data structures - a numerically-indexed array, or an object - I would think that a numerically-indexed array would make more sense as the first dimension, meaning you might end up with JSON like:

[
    {"group_id": 1, ... },
    {"group_id": 2, ... },
    ...
]

I think numerically-indexed arrays might also be used to hold the contacts and phone number data nested underneath the contact group.


You are treating all values from the result set as strings (the way they are represented by default when retrieving from mysqli_fetch_array()). Is this desired behavior, or does it make sense to cast primary key id's, for example, as integers?


Putting aside the problems noted above around LEFT JOINS and null. I would suggest that you consider working with each row of your results set as an object. IMO, this can make your code a lot cleaner and, in most cases, actually more truly represents what you are working with (an object representation retrieved from DB).

Taking this approach, as well as taking the approach noted on using numerically-indexed arrays for contact groups, contacts, and phone numbers, you might end up with something more like this:

$data = array();
$group = new stdClass();
$group->group_id = 0;
while ($row = mysqli_fetch_object($query)) {
    // build new contact group if needed
    if((int)$row->group_id !== $group->group_id) {
        // break existing reference between $group and $data
        // data in $data will remain after this dereferencing
        unset($group);
        $group = new stdClass();
        $group->group_id = (int)$row->group_id;
        $group->group_name = $row->group_name;
        $group->contacts = array();
        // build reference from group to location in $data
        // this allows you to simply work with $group here in loop
        // as opposed to $data[$index] which requires you to manually
        // track index values
        // taking similar apporach below for contacts as well eliminates
        // $data[$group_index]->contacts[$contact_index]->phone_numbers[]
        // types of references
        $data[] =& $group;

        // reset $contact
        unset($contact);
        $contact = new stdClass();
        $contact->id = 0;
    }
    // build new contact if needed
    if((int)$row->contact_id !== $contact->id) {
        // dereference contact
        unset($contact);
        // create new contact
        $contact = new stdClass();
        $contact->id = (int)$row->contact_id;
        $contact->name = $row->contact_name;
        $contact->phone_numbers = array();
        $group->contacts[] =& $contact;
    }
    // add phone number to current contact
    $phone = new stdClass();
    $phone->id = (int)$row->phone_id;
    $phone->number = $row->phone_number;
    $contact->phone_numbers[] = $phone;
}
unset($group, $contact, $phone);

You will note that taking this approach means that you are truly only building a single logical representation (a stdClass object) for each contact group and contact and simply adding phone numbers to the current contact and adding contacts to the current group. I think thinking in terms of objects here really makes the code more meaningful in a real-world sense than building out a convoluted associative array. Considering the JSON serialization converts both objects and associative arrays to object notation anyway, there really seems to be no value to using associative array here.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for your reply. Actually, I need the review for the code inside the loop. Is this code OK in PHP to create array? \$\endgroup\$ Commented Jan 25, 2017 at 8:38
  • \$\begingroup\$ @NhânTrần I am not sure I understand your question. My comments include thoughts on how your are building your data structure. \$\endgroup\$
    – Mike Brant
    Commented Jan 25, 2017 at 14:32
  • \$\begingroup\$ Example: $data[$row["group_id"]]["contacts"][$row["contact_id"]]["phone_number"][$row["phone_id"]]["number"] = $row["phone_number"]; does it acceptable? I use many [] in this. \$\endgroup\$ Commented Jan 26, 2017 at 3:17
  • \$\begingroup\$ @NhânTrần Again, I gave a suggestion on how I might approach it, as well as some potential pitfalls with your approach around handling nulls in the result set. Is your way "acceptable"? I guess that is up to you. \$\endgroup\$
    – Mike Brant
    Commented Jan 26, 2017 at 5:35

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.