Make me a better php programmer

PHP
1

Hello All,

I am an absolute PHP noob at present, I’m currently reading the Sitepoint book “Build your own database driven website…” and working through chapter 6 it seems to tie everything you have learnt previously into one large task.

Near the beginning of the chapter Kevin Yank(the author) suggests that if you are comfortable with multi purpose pages you can utilize that coding style vs. separate files to do specific tasks. I thought I was up to the challenge but I believe I have created the worlds most ugly code and even worse duplicated a shed load of variables.

It would mean a lot to me if someone more experienced could review my code and let me know how I can better myself.

I stumbled across one problem where I couldn’t produce the “edit” author form without breaking the “add” author screen, as the variables were not ready as yet. So my solution was to duplicate the search form and use conditional tags. I reckon that is totally wrong…


<html>
    <head>
        <title>Joke CMS</title>
    </head>
    <body>
        <h1>Manage authors</h1>
        <?php
        $dbcon = @mysql_connect('localhost','root');
        if (!$dbcon) {
            exit('<p>Unable to connect to the database server at this time.</p>');
        }
        if (!@mysql_select_db('ijdb')) {
            exit('<p>Unable to read database at this time.</p>');
        }
        if (isset($_REQUEST['editAuth'])):
            $authId = $_REQUEST['editAuth'];
            $preFillForm = @mysql_query("select * from author where id='$authId'");
                if ($preFillForm){
                    $preFillForm = @mysql_fetch_array($preFillForm);
                    $authName = $preFillForm['name'];
                    $authName = htmlspecialchars($authName);
                    $authEmail = $preFillForm['email'];
                    $authEmail = htmlspecialchars($authEmail);
                } else {
                    exit('<p>Unable to query database at this time:'. mysql_error() . '</p>');            
                }
        ?>
            <form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
                <p><label>Name: <input type="text" name="authName" value="<?php echo $authName; ?>" /></label></p>
                <p><label>Email: <input type="text" name="authEmail" value="<?php echo $authEmail; ?>" /></label></p>
                <input type="hidden" name="id" value="<?php echo $authId; ?>" />
                <p><input type="submit" value="Submit" /></p>
            </form>
            <p><a href="/db-sites/ch6/authors.php">Return to authors</a></p>
        <?php elseif(isset($_REQUEST['addAuthor'])): ?>
            <form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
                <p><label>Name: <input type="text" name="authName" /></label></p>
                <p><label>Email: <input type="text" name="authEmail" /></label></p>
                <p><input type="submit" value="Submit" /></p>
            </form>
            <p><a href="/db-sites/ch6/authors.php">Return to authors</a></p>        
        <?php else:
        if (isset($_REQUEST['id']) and isset($_REQUEST['authName'])) {
            $authName = $_REQUEST['authName'];
            $authEmail = $_REQUEST['authEmail'];
            $authId = $_REQUEST['id'];
            $sqlUpd = mysql_query("update author set name='$authName', email='$authEmail' where id='$authId'");
            if ($sqlUpd) {
                echo '<p>Author has been edited.</p>';    
            } else {
                exit('<p>Unable to query database at this time:'. mysql_error() . '</p>');        
            }
        } elseif(isset($_REQUEST['authName'])) {
            $authName = $_REQUEST['authName'];
            $authEmail = $_REQUEST['authEmail'];
            $sqlInsAuth = "insert into author set name='$authName', email='$authEmail'";
            if (@mysql_query($sqlInsAuth)) {
                echo '<p>Author has been submitted.</p>';    
            } else {
                exit('<p>Unable to query database at this time:'. mysql_error() . '</p>');        
            }        
        }
        if (isset($_REQUEST['delAuth'])) {
            $authID = $_REQUEST['delAuth'];
            $delAuth1 = @mysql_query("delete joke, jokecategory from joke, jokecategory where jokeid = joke.id and authorid ='$authID'");
            $delAuth2 = @mysql_query("delete from joke where authorid='$authID'");
            $delAuth3 = @mysql_query("delete from author where id='$authID'");
            if ($delAuth1 and $delAuth2 and $delAuth3) {
                echo '<p>Author has been deleted (inc. published jokes).</p>';        
            } else {
                exit('<p>Unable to query database at this time:'. mysql_error() . '</p>');                        
            }
        }
        $sqlRdTbl = @mysql_query("select * from author");
        if (!$sqlRdTbl) {
            exit('<p>Unable to query database at this time:'. mysql_error() . '</p>');    
        }
        ?>
        <table>
            <tr>
                <td><strong>Author name</strong></td>
                <td><strong>Edit</strong></td>
                <td><strong>Delete</strong></td>
            </tr>
        <?php
        while ($author = @mysql_fetch_array($sqlRdTbl)) {
            $aName = $author['name'];
            $aID = $author['id'];
            echo "<tr>" . "<td>" . $aName . "</td>" . "<td><a href=" . $_SERVER['PHP_SELF'] . "?editAuth=" . $aID . ">Edit</a></td>" . "<td><a href=" . $_SERVER['PHP_SELF'] . "?delAuth=" . $aID . ">Delete</a></td>" . "</tr>";
        }
        ?>
        </table>
        <p><a href="<?php echo $_SERVER['PHP_SELF'] . '?addAuthor=1'; ?>">Add new author</a></p>        
        <?php endif; ?>
    </body>
</html>

Many thanks!

2

Please change your

 tags to 
```php
 tags. It makes the code easier to read.
3

gingerj,

Looking at your code, the biggest change you could make to improve your work would be to separate your PHP code out into a separate file from your HTML code. The latest, 4th edition of the book you’re reading covers this in detail. Looking at your code, it looks like you’re working from a previous edition of the book.

4

Woohoo the author himself! :cool:

Also while I know its used by almost everyone I’ve never liked this format:


if ($Something){
   //Do something
} else {
   //Something else
}

I’ve always found it hard to read for some reason but this is always far easier:


if ($Something)
   {
   //Do something
   }
else
   {
   //Something else
   }

For some reason I find it far easier to read and it makes it a lot clearer for nested code too.

5

Thank you very much Kevin for your swift reply your observations are correct I reading the third edition.

I will look into purchasing the fourth post this book - would you know of any links or articles to cover off what you allude to in your recommendations? To give me a head start?

Regards,
James