1
\$\begingroup\$

I have written my own little auto-generated table class, and I was wondering if you guys can give me some tips on how to make this code more elegant and secure and maybe compact it so I use only one master function to bind everything when I call it out!

The DB connection file:

<?php
$host = 'localhost';
$db   = '***';
$user = '***';
$pass = '***';
$charset = 'utf8';

$dsn = "mysql:host=$host;dbname=$db;charset=$charset";

$settings = [
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
    PDO::ATTR_EMULATE_PREPARES   => false,
];

$pdo = new PDO($dsn, $user, $pass, $settings);

The class file:

<?php

include_once 'db.php';

class FoxyCRUD {

    /* Properties */
    private $conn;
    private $table;
    private $where;

    /* Get database access */
    public function __construct(PDO $pdo) 
    {
        $this->conn = $pdo;
    }

    /* Get database access */
    public function __destruct() 
    {
        $this->conn = null;
    }



    /* Fetch db.table -> column names & type */
    public function get_header_elements($table) 
    {
        // Set table name
        $this->table = $table;

        $query = $this->conn->prepare('SHOW FULL COLUMNS FROM '.$this->table);
        $query->execute();
        return $query->fetchAll(PDO::FETCH_ASSOC);
    }

    /* Fetch db.table -> column names & type */
    public function where() 
    {
        $query = $this->conn->prepare('SHOW FULL COLUMNS FROM '.$this->table);
        $query->execute();
        $check_where = $query->fetchAll(PDO::FETCH_ASSOC);
        return $check_where[0]['Field'];
    }

    /* Fetch all data */
    public function get_table_data() 
    {
        $query = $this->conn->prepare('SELECT * FROM '.$this->table);
        $query->execute();
        return $query->fetchAll();
    }

    /* Fetch specific data */
    public function get_specific_data($where) 
    {
        // Set where value
        $this->where = $where;

        $query = $this->conn->prepare('SELECT * FROM '.$this->table.' WHERE '.$this->where().' = '.$this->where);
        $query->execute();
        return $query->fetchAll();
    }   
}

And the index file:

<?php

include_once 'classes.php';

$foxy_crud = new FoxyCRUD($pdo);

?>

<table border="1">

    <?php 

    // Generate table header
    $columns = $foxy_crud->get_header_elements('login');

    foreach ($columns as $column_header => $column_value) 
    {
        echo '<th>'.$column_value['Field'].'</th>';
    }


    // Populate table with data
    $data = $foxy_crud->get_table_data();

    foreach ($data as $data_text) 
    {
        echo '<tr>';

            // Get data using generated table header names
            foreach ($columns as $column_data => $column_value) 
            {
                echo '<td>'.$data_text[$column_value['Field']].'</td>';
            }

        echo '</tr>';
    }

    ?>


</table>

<br>
<br>

<?php

    // Fetch specific data where ID
    $fetch_data = $foxy_crud->get_specific_data('2');

    // Fromating fields
    foreach ($columns as $column_header_type => $column_value) 
    {

        // Identify Primary ID
        $id_key = $column_value['Key'];

        // Generate label names
        $name = $column_value['Field'];

        foreach ($fetch_data as $data_text) 
        {
            $value = $data_text[$name];
        }

        $name = str_replace('_', ' ', $name);

        $name = ucwords($name);

        // Determine field type and maxlength
        list( $type, $max ) = explode ('(', $column_value['Type']);

        $max = explode (')', $max)[0];

        // Specify how to format each field type
        switch ($type) 
        {

            case 'int':
            $type = 'hidden';
            $disable = 'disabled';
            $placeholder = '';
            $br = '';
            break;

            case 'varchar':
            $type = 'text';
            $disable = '';
            $placeholder = $name;
            $br = '<br>';
            break;

            case 'datetime':
            $type = 'date';
            $disable = '';
            $placeholder = '';
            $br = '<br>';
            break;

            case 'tinyint':
            $type = 'tel';
            $disable = '';
            $placeholder = $name;
            $br = '<br>';
            break;

        }

        // Determine if field is type Password - only works if tablea header name is `password`
        switch ($name) 
        {

            case 'Password':
            $type = 'Password';
            $disable = '';
            $placeholder = $name;
            $br = '<br>';
            break;

        }

        // Determine if label text is from Primary ID -> hide
        switch ($id_key) 
        {

            case 'PRI':
            $label = '';
            break;

            default:
            $label = '<label for="'.$name.'">'.$name.' ('.$max.')</label><br>';
            break;

        }

        // Generate labels
        echo $label;


        // Generate form fields
        echo '<input type="'.$type.'" name="'.$name.'" id="'.$name.'" placeholder="'.$placeholder.'" value="'.$value.'" maxlength="'.$max.'" '.$disable.'>'.$br;

    }

?>

I know this is just the barebones, but I am trying to make this school project fully functioning, and I want to format the code as correctly and as simply as possible.

\$\endgroup\$
1
\$\begingroup\$

Honestly, there are too much issues to address.

First of all, your code is not safe from SQL injection, because of cargo cult prepared statements

Another kind of SQL injection is highly likely to happen too, as, due to the nature of the script, it would be natural to show the structure for the user-supplied table name which goes to the query unprotected.

FoxyCRUD::where() is mis-named. it should be called get_id_field(). And so get_specific_data() as well, it should be named find_by_id() instead. Besides, your convention is not very reliable, the id field is not necessarily the first one.

All other conventions are clumsy as well, such as for the password or int fields.

Why the main functionality is taken out from the class? Are you going to write the same code for the every table instead of just calling a method()?

You should take a look at PSR-2 for the code formatting guidelines.

And the most important issue: your question is a borderline off topic as your code is barely usable in the real life, while the rules require a fully functional code from you. So try to use this code for something useful and then come back.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your feedback. I will research what you have sent me! \$\endgroup\$ – user1286956 Oct 16 '18 at 8:19
  • \$\begingroup\$ Could you tell me how I can deal with the password? For the Primary key field and input, I can solve by determining where is the PRI value, but I don't know how to determine the same for the password column. \$\endgroup\$ – user1286956 Oct 16 '18 at 8:51
  • \$\begingroup\$ Well I should admit, it's OK to set the password type based on the filed name. It wouldn't do any harm. \$\endgroup\$ – Your Common Sense Oct 16 '18 at 9:06
  • \$\begingroup\$ Thank you very much awesome Your Common Sense :) \$\endgroup\$ – user1286956 Oct 16 '18 at 9:22

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.