0
\$\begingroup\$

As tim suggested, some changes to the database was necessary. I have posed an improved version of this question at Get title from database based on member country.


I have the following database:

Database sample information

Each member may have multiple countries (1:n) mapped in table country2memb. It's even possible that one member gets the same country multiple times (for example, Bernd got US twice).

Further, each member may have multiple titles (1:n) mapped in table title2memb. Each entry idRank has a name which can be found in title.

I want to get the first N members that have at least once the country "US" in table country2memb and I want to display all their titles.

Here is working code, using two JOINS and two where statements for N=2:

<?php

$db= new mysqli("localhost","root","","example");

$sql      ="SELECT DISTINCT m.name, m.id FROM member m
INNER JOIN country2memb c ON m.id = c.idMember AND c.country='US' LIMIT 2";

$result   = $db->query($sql);

while($zeile = $result->fetch_assoc()){
  $id = $zeile['id'];     
  echo $zeile['name'].": ";

  $sub_sql = "SELECT t.name FROM title t INNER JOIN title2memb tm ON
  t.id = tm.idRank AND tm.idMember='$id'";
  $sub_result   = $db->query($sub_sql);
  while($rank = $sub_result->fetch_assoc())
  {
    echo $rank['name']." ";
  }

  echo "<br>";
}


?>

And as a result I get (I had to use the DISTINCT keyword, otherwise I would get Bernd twice):

Bernd: Bee

Tom: Bear Dog

Is it really necessary to have 2 nested while clauses here?

\$\endgroup\$
3
\$\begingroup\$

Formatting

Your code has some formatting issues, reducing the readability of it:

  • spacing: your spacing around = seems arbitrary and you are missing spaces around ..
  • indentation: 2 spaces aren't really enough, use at least 4.
  • you should also indent the queries after a linebreak a bit more to increase readability.
  • your curly brackets are not consistent.

Security

Depending on who can add members and ranks, you may be vulnerable to XSS as the name is echoed unencoded. Even if this isn't a problem now because only you can add them, your requirements may change. Encoding input is always a good idea.

Also, depending on the type and changability of id you may be vulnerable to SQL injection. Always use prepared statements.

Structure

You should add functions to increase the maintainability - readability, reusability, changability, etc - of your code.

For example, selectMembersByCountry($country) or selectMemberRanksByID($id), which would then return an array of members and/or ranks.

Naming

zeile isn't a good name. First of all, your variable names shouldn't switch languages, and ideally they should all be in English.

But row wouldn't be much better. What is it really? Well, $result contains a list of members, right? So its name could reflect that, and then zeile could be member, which would give you: $member = $membersResult->fetch_assoc().

Simplify SQL

You could put all the logic directly into the query, but I don't think that that is necessary, so I'll leave that to someone else (although you may want to change your table structure first, see below).

I would say that it's fine the way it is. Especially when you extract the queries to functions, you would have well-defined and reusable functions.

Putting all the logic directly in the query is likely to be less readable and harder to maintain, and I doubt that it will give you a noticeable performance benefit (definitely not if you limit by 2).

Table Structure

  • Why do your mapping tables have their own id? This doesn't seem necessary.
  • For a 1:n relationship, you don't actually need a mapping table.
  • member-country doesn't actually seem to be a 1:n relationship, but n:m; A member can have multiple countries, and a country can have multiple members.
  • Because of the last point, you actually have duplications in your country2memb table (multiple entries of US for example). What you should do is have a member table, a country table, and a memberCountry table to map between them.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for this answer. Well I guess I need to do some checks how much the performance will improve, when another solution is given. To the 4 Table Structure questions: 1) You are right 2) Yes, you are right 3) You are right! 4) well I tried to simplify my original example which was a mapping between address and member, and a member could have had multiple addresses (1:n) where multiple addresses could have the same country. The way I coded it above was not really good - sorry for that. Thank you for your suggestions. \$\endgroup\$ – Adam May 22 '16 at 13:55

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.