Clumsy PHP5 code reading from an SQLite3 database

Question

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?

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. — MPelletier
– MPelletier, Commented Feb 20, 2012 at 0:29
For the second, does foreach in PHP mean each row or each column? — MPelletier
– MPelletier, Commented Feb 20, 2012 at 0:30
And do yourself a favour, list the columns you're getting explictly instead of doing "select *". — MPelletier
– MPelletier, Commented Feb 20, 2012 at 0:31
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. — qubyte
– qubyte, Commented Feb 20, 2012 at 2:52

pgraham · Accepted Answer · 2012-02-24 18:13:09Z

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.

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? — qubyte
– qubyte, Commented Mar 5, 2012 at 4:51
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. — pgraham
– pgraham, Commented Mar 13, 2012 at 17:47
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. — qubyte
– qubyte, Commented Mar 13, 2012 at 18:01

Stack Exchange Network

Clumsy PHP5 code reading from an SQLite3 database

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Clumsy PHP5 code reading from an SQLite3 database

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions