Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Yesterday I posted a question involving multiple nested queries. The queries pulled information from the database and created a directory listing of all employees. There are two many-to-many relationships involving 5 tables. For each employee they can have multiple job titles and multiple departments. This is the updated code:

$stmt = $mysqli->prepare("SELECT employeeId, firstName, middleName, lastName, suffix, profilePhoto, GROUP_CONCAT(DISTINCT jobName ORDER BY jobTitleId), GROUP_CONCAT(DISTINCT departmentName ORDER BY departmentId), GROUP_CONCAT(DISTINCT departmentURL ORDER BY departmentId) FROM employee 
INNER JOIN employee_has_jobTitle ON employeeId = employee_has_jobTitle.employee_employeeId 
INNER JOIN jobTitle ON employee_has_jobTitle.jobTitle_jobTitleId = jobTitleId 
INNER JOIN employee_has_department ON employee.employeeId = employee_has_department.employee_employeeId
INNER JOIN department ON employee_has_department.department_departmentId = departmentId
GROUP BY employeeId ORDER By lastName, firstName");

$stmt->execute();
$stmt->store_result();
$stmt->bind_result($employeeId, $firstName, $middleName, $lastName, $suffix, $profilePhoto, $jobName, $departmentName, $departmentURL);

//set up the list
echo "<ul class='plainList'>";

while($stmt->fetch()){

    echo "<li><div class='clearfix'><img src='/profileImages/$profilePhoto' alt='$firstName $lastName' class='directoryPhoto'>";

    $deptName = explode(",", $departmentName);
    $deptURL = explode(",", $departmentURL);
    $jobTitle = explode(",", $jobName);
    echo "<strong>$firstName $lastName</strong><br>";

    $counter = 0;
    foreach($deptName as $name) {
        //echo a preceding comma if not the first department
        if($counter > 0) echo ", ";

        echo "<a href='$deptURL[$counter]'>$name</a>";
        $counter++;
    }

    echo "<br>";

    foreach($jobTitle as $job) {
        echo "$job<br>";
    }

    echo "</div></li>";
}

//close the list
echo "</ul><br>";

If you check my other question, you can see that this is an improvement. However, I don't think the query and code are as elegant as they should be. Is there an obvious way to simplify the code and enhance performance that I'm not seeing?

UPDATE

Screenshot of the ERD for reference:

Simplified ERD for reference

share|improve this question
    
Have you thought about using concatenated subqueries instead. See hellotecho.com/… for an example. –  B2K Apr 16 at 20:31
    
@B2K That's what I ended up doing to get the job title and departments. Originally it was nested queries, but this question has the GROUP_CONCAT. –  Matthew Johnson Apr 16 at 21:01
    
I'm referring to doing group_concats in subqueries. See my answer below –  B2K Apr 16 at 21:13

2 Answers 2

in your while statement you are listing all sorts of information inside of one list item tag, I am not so sure this is intentional, it looks like it is going to be really messy. I think you should look into creating sub-lists there especially for the multiple Department names and Job Names. you want something like an XML Date File here, maybe I am overstepping my reviewer line and telling you to change the structure, but this is going to look really messy on a website I think.

something like this would be more convenient.

<li>
    <div class='clearfix'>
       <img src='/profileImages/$profilePhoto' alt='$firstName $lastName' class='directoryPhoto' />
       <strong>$firstName $lastName</strong> <!-- should probably be a span with styling hooks -->
       <ul>
           <li>Department1</li>
           <li>Department1</li>
       </ul>
       <ul>
           <li>Job1</li>
           <li>Job2</li>
       </ul>
    </div>
</li>

I don't know exactly what you want your finished product to look like, but this structure is far more solid than what your PHP is going to output, you should look into making it output something like this, because this is going a lot easier to work with for Styling and manipulating with PHP.

all of your image tags (<img>) need to be closed(<img />), so does your break tags (<br>,<br />)


From what you are saying it sounds like you want something more like this

<li>
    <ul>
        <li>
            <img src='/profileImages/$profilePhoto' alt='$firstName $lastName' class='directoryPhoto' />
        </li>
        <li>
            <strong>$firstName $lastName</strong> <!-- should probably be a span with styling hooks -->
        </li>
        <li>Department1, Department1</li>
        <li>Job1, Job2</li>
    </ul>
</li>

I removed the Div tag and some of the other stuff just so we could visualize this better. this is a little more organized than what you are going to get with your current code. it will still be easier to deal with.

As Far as whether or not to do another list for the Department Names and Job Titles, I would go with yes, even if they only have a single item, it will be easier to navigate the ones that do have them, and easier to manipulate the data, it is more maintainable I think.


I mentioned still using nested lists for the Department Names and Job Names, you should still have a title for the list item that is holding them, so you know what they are, but I will leave that up to you

<li>
    <ul>
        <li>
            <img src='/profileImages/$profilePhoto' alt='$firstName $lastName' class='directoryPhoto' />
        </li>
        <li>
            <strong>$firstName $lastName</strong> <!-- should probably be a span with styling hooks -->
        </li>
        <li>
            <ul>
                <li>Department1</li>
                <li>Department1</li>
            </ul>
        </li>
        <li>
            <ul>            
                <li>Job1</li>
                <li>Job2</li>
            </ul>
        </li>
    </ul>
</li>

so here is what that would look like with out the titles for the sub lists

share|improve this answer
    
The img and br tags no longer need to be closed in HTML5. It actually displays as intended, though the code does get messy. My primary reason behind not having department and job title as sub lists is that for the vast majority of people they only have a single title and department. With that, should they still be sub-lists even though the bulk would only have a single li? The "old" directory listing has all departments on one line, and then job titles listed on separate lines. As of now, you can't visually differentiate between the "old" and "new" directories. –  Matthew Johnson Apr 16 at 16:23
    
The li setup I settled on because I'm "listing" employees, though previously it was all done with div elements. Should I revert to simple div elements for simplicity sake? –  Matthew Johnson Apr 16 at 16:24
1  
I think that closing your tags is something that you should always do no matter what, it is a good habit and as far as I know it is still standard for every other Markup language in the XML Style Family (HTML, XHTML, XML, HTML5, etc) it's my personal preference too, even if I don't have to. I wouldn't go back to div's your list presents the data the way it is supposed to be presented. –  Malachi Apr 16 at 16:38
    
Many thanks for your input, and I can understand the "good habit". HTML5 allows attributes to go unquoted, and I just can't force myself to do that! :) –  Matthew Johnson Apr 16 at 16:41

Using subselects may help alleviate the issues requiring distinct grouping statements, and also will prevent the inner joins from removing employees who haven't been assigned titles or departments (although you could also use outer joins for that).

 SELECT employeeId, firstName, middleName, lastName, suffix, profilePhoto,
       (SELECT  GROUP_CONCAT(jobName ORDER BY jobName) 
          FROM jobTitle 
          INNER JOIN employee_has_jobTitle 
                  ON employee_employeeId = employeeId 
                 AND jobTitle_jobTitleId = jobTitleId) as jobs,        
       (SELECT GROUP_CONCAT(CONCAT(departmentName,' ',departmentURL) ORDER BY departmentName)
          FROM department
          INNER JOIN employee_has_department
                  ON employeeId = employee_employeeId
                 AND department_departmentId = departmentId) as Departments 
   FROM employee
   ORDER BY lastName, firstName

One last point to note is that formatting your query as I have done can be an immense help to understanding it, especially when you're returning to code you've written years before.

share|improve this answer
    
Your correct, I like that formatting MUCH better. There's an error in this statement. It doesn't recognize the employeeId in the ON clause of the first sub-query. Do the sub-queries run before the first query? I don't know how you would narrow the job titles down to just the right ones without adding employeeId in a WHERE clause in the sub-queries. I added a screenshot of a simplified ERD to show the table setup. Definitely will be reformatting my SQL statements though! –  Matthew Johnson Apr 16 at 21:41
    
@MatthewJohnson You can add the table name to it. employee.employeeId –  B2K Apr 16 at 21:42
    
That's what I tried originally, but it just updated the error to say unknown column employee.employeeId instead of just employeeId. Should there be another INNER JOIN to bring the employee table too? –  Matthew Johnson Apr 16 at 21:46
    
@MatthewJohnson Try assigning an alias to every table, then prefixing each field with the appropriate alias. That should resolve any issues. No, the join for the employee table is the selected row. –  B2K Apr 16 at 22:04

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.