Code Help Please

[johnb]

Hi

Fairly new to PHP and have the following code which should simply output into 3 columns - I have checked the query which does work - Pointers appreciated

// Create connection
$conn = new mysqli($servername, $username, $password, $dbname);

// Check connection
if ($conn->connect_error) {
	die("Connection failed: " . $conn->connect_error);
} 



$sql = "SELECT * FROM duration_matched WHERE groupm = '".$coincidence ."'";
$result = $conn->query($sql);


// set up loop counter
$col_count = 0;

// start table and first tr
echo '<table><tr>';

while ($row=mysql_fetch_array($result)) {
   // if you have output 3 cols then end tr and start a new one
   if ($col_count == 3) {
      echo '</tr><tr>';
      //  and reset the col count
      $col_count = 0;
   }



   // always output the td
   echo '<td>' . $row['image'] . '</td>';
  // and count the column
  $col_count++;
}

// then close off the last row and the table
echo '</tr></table>';



$conn->close();

[Wava66]

1. I am not sure where the $coincidence variable originates but if it is from user input you are open to SQL injection. Look up how to use mysqli parameters and the prepare statement. Also look into using PDO for your mysql access.
2. Even if the $coincidence variable does not originate from user input use parameters anyway. Get used to using parameters so that your queries are safer.
3. Include just the columns you wish to retrieve. Since you are only using the image column I would limit the SQL to just that column. It will help make the query faster with less overhead.
4. Look at the printf function that should simplify your output.
5. On your die function be careful about giving too much information.‘Connection Failed’ should be enough information for the users of your site. I understand that the extra information helps you debug, but it also gives others information about your back end. Now connect_error probably does not give out too much information, but I like to limit the information the users will se.