Efficiency Question

[947740]

I've been doing php off and on for a couple years now.  I can do a lot with it...but I do not know if the way I do it is the best way available.  So what I'm going to do here is post some code along with a basic idea behind it and ask if it is one of the better ways to do it.  If it's not, any input as to how to better do it would be greatly appreciated.

 

As to what this code does...the first section checks if the form has been submitted.  If it has, then it processes the data and creates a new link.

 

the included file config.php contains the constants used to connect to the database, as well as including a functions.php file, (where the getHeader() and getFooter() functions are located.)

 

The second section (only processed if a form has not been submitted) displays the form to create a new link.  If you want to see what the final product looks like it can be found here.

 

I understand my request here is probably different than usual, as the script does what it's meant to do.  I'm just concerned about my practices and the efficiency in which I do things.  Thanks ahead of time.

 

Anyways...the php code:

 

<?php

// Show all errors

error_reporting(E_ALL);
ini_set('display_errors', '1'); 

session_start();

define('ROOT', "../");

require_once(ROOT . "includes/config.php");

$mysqli = new mysqli(HOST,USERNAME,PASSWORD,DATABASE);

// Form was submitted, we need to create the link
if(isset($_POST['submit'])) {
// Sanitize data
$ln = $mysqli->real_escape_string($_POST['link_name']);
$color = $mysqli->real_escape_string($_POST['color']);
$pl = $mysqli->real_escape_string($_POST['parent_link']);
$article = $mysqli->real_escape_string($_POST['article']);
$url = $mysqli->real_escape_string($_POST['url']);
	if($url != "") {
	$article = "";
	}

$admin = $_POST['admin'];
	if($mysqli->query("INSERT INTO links (link_name,color,parent_link,article,admin,url) VALUES ('$ln','$color','$pl','$article','$admin','$url')")) {
	$_SESSION['msg'] = "Link added.";
	}
	else {
	$_SESSION['msg'] = "Link was not added: " . $mysqli->error;
	}

header("Location: " . ROOT);
exit();
}

$header = getHeader(CREATE_LINK_ID);

echo $header;

// Get list of articles

$result = $mysqli->query("SELECT * FROM articles WHERE nolink = 0");
$articlebox = "<select name='article'>";
$articlebox .= "<option value=''>None";
while($row = $result->fetch_row()) {
$articlebox .= "<option value='{$row[0]}'>{$row[1]}";
}
$articlebox .= "</select>";

// Get list of links

$result = $mysqli->query("SELECT ID,link_name FROM links WHERE parent_link = 0");
$linkbox = "<select name='parent_link'>";
$linkbox .= "<option value='0'>None";
while($row = $result->fetch_row()) {
$linkbox .= "<option value='{$row[0]}'>{$row[1]}";
}
$linkbox .= "</select>";

echo <<<END
<div id='center'>
<div id='floaty'>
<form name='form' action='create.php' method='POST'>
Link name: <input type='text' name='link_name' />
Color: <input type='text' name='color' value='#ffffff' />
<br />
Parent link: $linkbox

Admin Only: <select name='admin'>
<option value='0'>No
<option value='1'>Yes
</select>
<br />
URL: <input type='text' name='url' />
Article: $articlebox
<br />
<input type='submit' name='submit' value='Create Link' />
</form>
</div>
</div>
END;

$footer = getFooter();

echo $footer;

?>

 

[947740]

I would really appreciate some input...even if you think no improvements could be made, letting me know that would be great.

 

Thanks.

[roopurt18]

// Form was submitted, we need to create the link
if(isset($_POST['submit'])) {

See this thread: http://www.phpfreaks.com/forums/index.php/topic,167898.15.html

 

error_reporting(E_ALL);
ini_set('display_errors', '1'); 

session_start();

define('ROOT', "../");

require_once(ROOT . "includes/config.php");

$mysqli = new mysqli(HOST,USERNAME,PASSWORD,DATABASE);

I imagine the head of all your files looks like that.  Seems like a lot of redundant typing to me.  Why not use mod_rewrite and send all requests through index.php.  Then you can perform those steps in index.php and later include() the necessary page based on $_SERVER['REQUEST_URI'].

 

$ln = $mysqli->real_escape_string($_POST['link_name']);

You're not checking that 'link_name' is present in post and opening your script up for errors.  You should do something like:

$link_name = post_val( 'link_name' );

// And if link_name has a default you can use then you can do:
$link_name = post_val( 'link_name', 'Untitled Link' );

function post_val( $name, $default = false ) {
    return array_key_exists( $name, $_POST ) ? $_POST[ $name ] : $default;
}

 

$_SESSION['msg'] = "Link was not added: " . $mysqli->error;

I imagine this message is displayed to the user.  Unless you're the only one using this, there's never a good reason to display database message errors to users.  Instead you should log the database error so you [the developer] can see what happened.  The user should get something more generic like Unable to add link at this time.

 

$result = $mysqli->query("SELECT * FROM articles WHERE nolink = 0");

You should avoid select * and instead list the columns you are expecting by name.  This prevents the database from passing unnecessary data over the socket, which speeds things up.  And if this table grows to have more columns and many, many rows select * can negate the benefit of any indexes you place on the table.

 

$articlebox .= "<option value=''>None";

You're not closing any of your option tags.  If this page is supposed to be XHTML strict [which is ideal in most circumstances], then not closing tags makes it invalid markup.  Invalid markup will adversely affect any CSS or JavaScript you write.  Often times JavaScript (dealing with the DOM) or CSS rules don't work as expected and it's often because of bad markup.  But people don't realize this so they write 5 times more CSS or JavaScript to work around the problem instead of just closing the necessary tags.

 

$articlebox .= "<option value='{$row[0]}'>{$row[1]}";

What's $row[0]?  What's $row[1]?  That's not very descriptive.  Oh sure, today they're name and description.  But what happens if you add a column before description?  Then $row[1] is no longer description and your code breaks.  Instead why not use:

while( $row = $result->fetch_object() ) {
  echo $row->id . ' ' . $row->name . PHP_EOL; // assuming columns are id and name
}

fetch_object() is the best!

 

"<option value='{$row[0]}'>{$row[1]}";

Why are you not using htmlentities()?  Should be:

$row[0] = htmlentities( $row[0], ENT_QUOTES );
$row[1] = htmlentities( $row[1], ENT_QUOTES );
"<option value='{$row[0]}'>{$row[1]}";

 

<form name='form' action='create.php' method='POST'>

I'm pretty sure attributes are supposed to be lower-case nowadays.

 

$footer = getFooter();

echo $footer;

Could be

echo getFooter();

[roopurt18]

One last thing.  All of that free-floating code in the global namespace, it gives me bad feelings.  I like to really, really make sure my pages and their variables are completely self-contained.

 

Therefore I usually define a base Page class with common functions like:

redirect( $url, $msg = null ) // redirect to $url, optionally save a message in $_SESSION to redirect page to display / use

get_session_message() // returns saved session message

post_val( $name, $default ); // retrieves from $_POST with optional default

query_val( $name, $default ); // retrieves from $_GET with optional default

is_post() // returns true if post page... i.e: return count( $_POST ) > 0

get_db() // returns handle to database

db_escape( $val ) // escapes database value so I don't have to

markup_escape( $val, $entities = true, $striptags = true ) // will strip tags and htmlentities a value

abstract function run(); // derived pages must provide this method

 

Now your page can look like this:

<?php
//TODO
// Page should be in a different file
class Page {
    protected function post_val( $name, $default = false ) {
        return array_key_exists( $name, $_POST ) ? $_POST[ $name ] : $default;
    }

    protected function get_db() {
        static $db = null;
        if( $db === null ) {
            // TODO Change path to correct one
            include( dirname( __FILE__ ) . '/db.config.php' );
            $db = new mysqli( HOST, USERNAME, PASSWORD, DATABASE );
        }
        return $db;
    }

    protected function insert_query( $table, $values ) {
        $db = $this->get_db();
        $cols = array_keys( $values );
        foreach( $cols as $key => $col ) {
            $cols[ $key ] = "`{$col}`";
        }
        foreach( $values as $col => $value ) {
            $values[ $col ] = $db->real_escape_string( $value );
        }
        return "
            insert into `{$table}`
            ( " . implode( ', ', $cols ) . " ) values
            ( " . implode( ', ', $values ) . " )
            ";
    }

    protected function log( $msg ) {
        // TODO Write $msg to a log file of your choosing.
    }
}

class CreateLinkPage extends Page {

    public function run() {
        if( $this->is_post() ) {
            $vals = array();
            $vals[ 'link_name' ] = $this->post_val( 'link_name' );
            $vals[ 'color' ] = $this->post_val( 'color' );
            $vals[ 'parent_link' ] = $this->post_val( 'parent_link' );
            $vals[ 'article' ] = $this->post_val( 'article' );
            $vals[ 'url' ] = $this->post_val( 'url' );
            $vals[ 'admin' ] = $this->post_val( 'admin' );
            if( $vals[ 'url' ] != '' ) {
                $vals[ 'artucle' ] = '';
            }
            $insert_qry = $this->insert_query( 'links', $values );
            $db = $this->get_db();
            if( $db->query( $insert_qry ) ) {
                $this->redirect( 'new_url.php', "Link added successfully." );
            }else{
                $this->log( $db->error );
                $this->redirect( 'new_url.php', "Unable to add link at this time." );
            }
        }

        $html = '';
        // TODO rest of your code for non-post
        echo $html;
    }

}

$pg = new CreateLinkPage();
$pg->run();
?>

[947740]

Thanks A LOT roopurt.  That's the kind of insight I was looking for.  I wasn't even expecting that much, but it's really appreciated.

 

I apologize for the delayed response, but I've had a lot of non-programming stuff bogging me down recently.  Hopefully I can get back on the ball here and knock this site out!

 

Cheers.