2
\$\begingroup\$

I'm working with SQL Server 2008 and wanted to make table builder, similar to Laravel's migration classes. I would like to be able to expand this down the road but the main purpose of this code will be to initialize the database. It will likely only be run a few times in production, but likely more for development work.

The main interface should be easily accessible for creating and dropping tables. I was thinking of something like this:

(NOTE: I can't use autoloading on this project for reasons)

/* Snipped none abstracted class */

I felt creating class for each table like laravel's migrations was a bit overkill, so I simplified it to a single Class that has the PDO instance and configuration passed in. Any advice on simplifying queries, better abstractions, or glaring security flaws would be appreciated!

// db-init.php
/* Generate PDO instance */

/**
 * Create User Table
 */
$UserTable = new CreateTable($pdo, 'Employee');
$UserTable->up([
    "userId INT NOT NULL",
    "firstName VARCHAR(32) NOT NULL",
    "lastName VARCHAR(32) NOT NULL",
    "email VARCHAR(64) NOT NULL",
    "password VARCHAR(72) NOT NULL DEFAULT '!test@12345'",
    "PRIMARY KEY (userId)",
]);
$UserTable->createIndex([
    'userId', 
    'email',
]);

Abstracted table class

<?php

class CreateTable {

    protected $db;
    protected $table;

    /**
     * Use dependency injection to pass in dependencies (ie. pdo instance)
     * and core configurations (ie. table name).
     *
     * @param PDO $pdo
     * @param string $table
     */
    public function __construct(PDO $pdo, string $table) {
        $this->db = $pdo;
        $this->table = $table;
    }

    /**
     * Creates table if it doesn't already exist
     *
     * @return void
     */
    public function up(array $columns) {

        if ($this->tableExists()) {
            echo "`{$this->table}` table already exists: create table aborted.<br>";
            return;
        }

        $query = "CREATE TABLE {$this->table} ( " . implode(',', $columns) . ")";
        $statement = $this->db->prepare($query);
        $statement->execute();
    }

    /**
     * Drops table if it exists
     *
     * @return void
     */
    public function down() {

        if (!$this->tableExists()) {
            echo "`{$this->table}` table does not exist: drop aborted.<br>";
            return;
        }

        $query = "DROP TABLE {$this->table}";
        $statement = $this->db->prepare($query);
        $statement->execute();

        echo "`{$this->table}` table was dropped successfully<br>";
    }

    /**
     * Checks this table exists
     *
     * @return boolean
     */
    protected function tableExists() {
        $query = "SELECT DISTINCT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME = '{$this->table}'";
        $statement = $this->db->prepare($query);
        $statement->execute();
        $results = $statement->fetchAll();

        if (empty($results)) {
            return false;
        }

        return true;
    }

    /**
     * Undocumented function
     *
     * @param array $indexes
     * 
     * @return void
     */
    public function createIndex(array $indexes) {

        foreach($indexes as $index) {

            $index_id = strtolower($this->table . '_' . $index);

            if ($this->indexExists($index_id)) {
                continue;
            }

            try {
                $query = "CREATE INDEX {$index_id}
                          ON {$this->table} ({$index})";

                $statement = $this->db->prepare($query);
                $statement->execute();
            } catch (PDOException $e) {
                echo "There was a problem indexing {$this->table} ({$index}). " . $e->getMessage();
            }
        }

    }

    /**
     * Check if table index already exists
     *
     * @param [string] $index_id
     * 
     * @return boolean
     */
    protected function indexExists($index_id) {
        $query = "SELECT *  FROM sys.indexes 
                  WHERE name='{$index_id}' 
                  AND object_id = OBJECT_ID('[dbo].[{$this->table}]')";

        $statement = $this->db->prepare($query);
        $statement->execute();
        $results = $statement->fetchAll();

        if (empty($results)) {
            return false;
        }

        return true;
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ There is a bug in your code, a logical one in class CreateUserTable. In your method up() it says Creates table if it doesn't already exist However you bail if the table exists returns false if (!$this->tableExists()) return false. I believe this should bail if the table exists (returns true). It's done correctly in class CreateTable. This could also be done using IF NOT EXISTS such as CREATE TABLE IF NOT EXISTS {$this->table} (...) But you won't be able to do the output then. Just wanted to point those 2 things out. \$\endgroup\$ Jun 19, 2019 at 22:01
  • \$\begingroup\$ I've been wanted to build a table creator that uses XML as the config file for the table. Basically build the table schema using XML and then have a class parse it and create/modify the table. One of these days. \$\endgroup\$ Jun 19, 2019 at 22:08
  • \$\begingroup\$ @ArtisticPhoenix Good catch. Yeah that was a mistake. I added the bail checks right before posting and missed that the up() one was reversed initially. \$\endgroup\$
    – Dan
    Jun 20, 2019 at 12:39

1 Answer 1

3
\$\begingroup\$

This is code review, we didn't need to see your non-abstracted class since it doesn't need reviewing.

I will not do a complete review of the abstracted class, but I have a few comments and tips.

First of all, your class is called CreateTable, but this class can also destroy a table. That is the opposite of what it says on the tin. I think a better name would be ManipulateTable.

$employeeTable = new ManipulateTable($pdo, 'Employee');

Which is just another way of saying: "I want to work on the table 'Employee' now.".

I don't like the lump methods up() and down(). They are not very flexible or abstract. I would, instead, use methods to create or delete one column:

$employeeTable->deleteColumn("password")
              ->commitToDatabase();

$employeeTable->createColumn("passwordHash", "VARCHAR(64)")
              ->commitToDatabase();

After all, columns are the most important things when manipulating tables, so you want to be able to manipulate them individually. The commitToDatabase() method is used to update any changes to the database. To create multiple columns you would do:

$employeeTable->createColumn("userId", "INT")
              ->createColumn("firstName", "VARCHAR(32)")
              ->createColumn("lastName", "VARCHAR(32)")
              ->createColumn("email", "VARCHAR(64)")
              ->createColumn("password", "VARCHAR(72)", false, "!test@12345")
              ->createColumn("PRIMARY KEY (userId)")
              ->createPrimaryIndex("userId")
              ->commitToDatabase();

In other words, I would take your approach one step further. The ManipulateTable class is now more than a wrapper around the "CREATE TABLE" statement and has become a general purpose class for manipulating tables. It actually makes it easier to work with tables. I could, for instance, use it to add a column to an existing table. You can't do that with your class because you can only do up([columns]) or down().

As for the code in your class: Your up() method doesn't return anything. If a table already exists it does nothing, and the user of that method will never know that. Why not return false in that case and true when the table was correctly created?

I also think that:

if (empty($results)) {
    return false;
}
return true;

is the same as:

return !empty($results);

you use this construction twice.

\$\endgroup\$
3
  • \$\begingroup\$ Good remarks. I noticed the same thing with the class adopting more responsibilities beyond table creation, so I simplified the class name to just Table. I think you make a good point about just adding a column editor to complete the class. For the return !empty() part, I felt while it is more verbose to write it my way, the logic is more straight forward for individuals across skill levels to understand what's happening, albeit marginally, and is also open for extension. \$\endgroup\$
    – Dan
    Jun 20, 2019 at 12:55
  • 1
    \$\begingroup\$ I made the assumption you were using MySQL, but you're using SQL Server 2012, so I'll delete those last remarks about the schema tables. My mistake. \$\endgroup\$ Jun 20, 2019 at 13:19
  • 1
    \$\begingroup\$ Of course, once you have something like createColumn() you can still make a createColumns() method, that accepts an array of column definitions, and calls createColumn() multiple times. \$\endgroup\$ Jun 20, 2019 at 13:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.