My boss gave me this code that an applicant gave him. He wants me to write up a little email to him about what is wrong/shouldn't be done, or about what i just dont like about the code.

What do you guys see about it ?

script 1:



<?php
$link = mysql_connect("sitesite.org", "user", "pass");
mysql_select_db("data");

$query = "SELECT * FROM Meeting_Summary";
$result = mysql_query($query);

print "<table>\n";
while ($line = mysql_fetch_array($result, MYSQL_ASSOC)) {
print "\t<tr>\n";
foreach ($line as $col_value) {
print "\t\t<td>$col_value</td>\n";
}
print "\t</tr>\n";
}
print "</table>\n";

mysql_free_result($result);

mysql_close($link);
?>


script 2



<?php
$link = mysql_connect("sitesite.org", "user", "pass");
mysql_select_db("data");

$query = "SELECT * FROM QandA";
$result = mysql_query($query);

print "<table>\n";
while ($line = mysql_fetch_array($result, MYSQL_ASSOC)) {
print "\t<tr>\n";
foreach ($line as $col_value) {
print "\t\t<td>$col_value</td>\n";
}
print "\t</tr>\n";
}
print "</table>\n";

mysql_free_result($result);

mysql_close($link);
?>


The would be running on a site called 'sitesite.org' so the first thing i would think would be that the guy is using 'sitesite.org' as the server instead of using 'localhost'

Not necessarily as it depends on whether the mysql server is hosted on the localhost or not.

In large corporate sites you rarely host the database on the same machine as the web server.

There is nothing wrong in the logic of the code but I wouldn't have done it quite like that, rather


$link = mysql_connect("sitesite.org", "user", "pass");
mysql_select_db("data");


$result = mysql_query ("SELECT * FROM Meeting_Summary");

if (!$result) {
print mysql_error();
}

print "<table>\n";
if ($row = mysql_fetch_array($result)) {
do {
print "\t<tr>\n";
$col_value = $row[0];
print "\t\t<td>$col_value</td>\n";
print "\t</tr>\n";
} while($row = mysql_fetch_array($result));
}
print "</table>\n";

mysql_free_result($result);

mysql_close($link);


Also he doesn't have any error checking to catch if the database connection has failed or not (although I haven't included it in mine above either).

$link = mysql_connect("sitesite.org", "user", "pass");

The two things that I noticed right away:

1) As mentioned above, there is no error handling if the database doesn't connect, has an error in SQL syntax or returns no matching rows.

2) Since it's a snippet, this may not apply. I always recommend putting all database connection info into an include file and using constants to define the server, database, user and password. That way if you do end up changing servers or logins down the road, you're changing a few lines in one file instead of several blocks of code in dozens or hundreds of files.

If the database server is also on "localhost", it's usually faster to use localhost (socket to MySQL) than ports (3306).

Though, as was also mentioned before, many times the database is on a seperate machine.

1. Don't connect to sitesite.org -- connect to an IP address, or preferably to a local socket. Otherwise you're vulnerable to DNS attacks -- both denial of service (causing the script to hang around waiting for a timeout) and hijacking (causing the script to connect to the wrong host).

The far more serious problems, however, are
2. PHP, and
3. MySQL.

He's using a sledgehammer to crack a nut here; what he should do instead is have the table preconstructed in a file on disk which he can just `cat` out.

Neither example is commented, also you should not hard code your database login and password information into your scripts, it is not terrible to do it for small scripts but for larger programs if you hard code it and they change you need to update a lot of code.

Neither example does error checking, also neither example checks to make sure there is at least one record, while this will not result in a PHP error the scripts will display the table even if there is no information, logic to display a message like No Records Found would be appropiate.

There is only one mistake in the script which is that you are directly printing the values stored in the fields in the context of an HTML file. Before you do that, you must make sure that all HTML special characters are properly converted.

That is a mistake but not the only one, lack of error handling is a big mistake and so is not commenting your code :)

I wouldn't have posted again but I want to make it clear that both those are big mistakes.

Originally posted by Quickfoot
That is a mistake but not the only one, lack of error handling is a big mistake and so is not commenting your code :)

Lack of error handling, absolutely. But that code doesn't need comments.

All code needs comments, whether the code needs comments to describe what it is doing or not.

Especially if you are submitting code for a review, not commenting your code shows bad programming style.

A programmer who does not comment review code is not likely to comment an application very well.

Code doesn't NEED comments, but it's often useful, especially if

1) Someone else is going to need access the source code to develop

2) Refresh your own memory as to why you did something in a specific way.