4
\$\begingroup\$

Some time ago I wrote myself a simple blog. The index page is generated with PHP5, using an SQLite3 database via the variable id handed to it via the URL.

The SQLite database contains a table called pages. The id refers of course to the id intergers in this table. Other columns are title, body, time and timezone.

The title and body are text as you'd expect, the timezone is usually "Asia/Tokyo", and the time is seconds from the epoch (I just use date +%s, when I add an entry, nothing fancy).

I use the following code to get these variables from a table, and fill in a generic page with them.

<?php
    // Open the database.
    $db = new PDO("sqlite:pagesdatabase.db");
    // Is id set? If not get id of last page and put it in $id.
    if (!isset($_GET["id"])) {
        foreach ($db->query("select max(id) from pages") as $lastid) {
            $id = $lastid[0];
        }
    }
    // If id is set, get the value and put it in $id.
    else {
        $id = $_GET['id'];
    }
    // Using the id number, grab the title, body, time and timezone of post.
    foreach ($db->query("select * from pages where id = $id") as $page) {
        $title = $page['title'];
        $body = $page['body'];
        $time = $page['time'];
        date_default_timezone_set($page['timezone']);
    }
?>

This code works, but seems rather clumsy to me. For example, foreach appears twice, but shouldn't be necessary. To be honest, it's been a while since I wrote this code, and for some reason I had trouble using sqlite_open and other more obvious functions. Can someone help me write the above in a more terse way?

\$\endgroup\$
  • \$\begingroup\$ I don't know PHP, but I can tell you the first foreach is useless. A max query like that will return the same single value every call until an insert or an update changes the table. \$\endgroup\$ – MPelletier Feb 20 '12 at 0:29
  • \$\begingroup\$ For the second, does foreach in PHP mean each row or each column? \$\endgroup\$ – MPelletier Feb 20 '12 at 0:30
  • \$\begingroup\$ And do yourself a favour, list the columns you're getting explictly instead of doing "select *". \$\endgroup\$ – MPelletier Feb 20 '12 at 0:31
  • \$\begingroup\$ Be fair, I do state that the foreach should not be necessary. As for your last two comments, it doesn't matter since there's only one element. \$\endgroup\$ – qubyte Feb 20 '12 at 2:52
  • \$\begingroup\$ Sorry, I missed that statement. \$\endgroup\$ – MPelletier Feb 20 '12 at 3:46
3
\$\begingroup\$

You are right, the foreaches can be eliminated. The first foreach which grabs the highest id, can be eliminated by taking advantage of the fact that the query will only ever return a single cell and using the PDOStatement::fetchColumn() method. Since page ids are unique, the second foreach can be removed because the second query will only ever return a single row.

An additional problem is that you are not sanitizing user input in your SQL. This is easy to fix with PDO by using a parameterized statement:

<?php
// Open the database.
$db = new PDO("sqlite:pagesdatabase.db");

// Is id set? If not get id of last page and put it in $id.
if (!isset($_GET["id"])) {
    $id = $db->query("select max(id) from pages")->fetchColumn();
}
// If id is set, get the value and put it in $id.
else {
    $id = $_GET['id'];
}

// You may consider doing an additional check here in case there are no pages, in which
// case id will be null
if ($id === null) {
    // ...
    exit;
}

// Using the id number, grab the title, body, time and timezone of post.
$page = $db->prepare("select * from pages where id = :id")
           ->execute(array('id' => $id))
           ->fetch();

// A check for an invalid id should be made here
if ($page === null) {
    // ...
    exit;
}

$title = $page['title'];
$body = $page['body'];
$time = $page['time'];
date_default_timezone_set($page['timezone']);
?>

I have only quickly prepared this answer from experience and have not actually run the code above. It should be enough to give you an idea but may have syntax errors. If it does, comment on the post and I will fix it. Also, if reducing this code to the fewest lines possible is your goal, setting $id can be accomplished with a ternary and the actual retrieval of the page along with the error checks can be encapsulated in a function or a class.

<?php
// Open the database.
$db = new PDO("sqlite:pagesdatabase.db");

// Determine if a page id is provided, if not use the most recent page
$id = isset($_GET['id'])
  ? $_GET['id']
  : $db->query("select max(id) from pages")->fetchColumn();

try {
    $page = Page::fetch($id, $db);

    $title = $page->getTitle();
    $body = $page->getBody();
    $time = $page->getTime();
    date_default_timezone_set($page->getTimezone());

} catch (Exception $e) {
    // This exception should be thrown by Page::fetch if no pages exist or an invalid
    // id is given
    // ...
    exit;
}
?>

If this is preferred I will leave the class definition to you.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer. I haven't ignored it! Is there a particular reason that you have used PDO over SQLite specific functions? i.e. is there an advantage of one over the other? \$\endgroup\$ – qubyte Mar 5 '12 at 4:51
  • \$\begingroup\$ I have simply followed what you posted in your original example. Personally I only ever use PDO since I prefer the OO interface and because it provides parameterized queries which makes escaping user input much easier/safer. Not sure about the performance difference between the two. \$\endgroup\$ – pgraham Mar 13 '12 at 17:47
  • \$\begingroup\$ This is not a high volume site. At most a few hits a day. I'm not overly concerned with performance, but I like my code to be a mixture of clear and uncluttered. \$\endgroup\$ – qubyte Mar 13 '12 at 18:01

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.