Phonebook - a small PHP project

Question

I'm new in PHP, MySQL and OOP, so I tried to write a small project to learn things better. The Pponebook should contains contacts which can be edited, deleted, added, sorted and it has pagination, too. Search is also available. This is my first project in PHP and I am using OOP for the first time, so I am sure that my code is complete mess.

Contact:

<?php
class Contact
{
    private $name;
    private $phone ;
    private $address;
    private $notes ;

    private function isValid($var, $min_limit)
    {
        if(strlen(trim($var))>=$min_limit)
        {
            return true;
        }
        return false;
    }

    public function __construct($name, $phone, $address='', $notes='') 
    {
        if($this->isValid($name, 3)&& $this->isValid($phone, 3))
        {
            $this->name = addslashes(trim($name));
            $this->phone = addslashes(trim($phone));
            $this->address = addslashes(trim($address));
            $this->notes = addslashes(trim($notes)); 
        }
        else if (!$this->isValid($name, 3))
        {
            throw new Exception('The name is too short.');
        }
        else if (!$this->isValid($phone, 3))
        {
            throw new Exception('The phone number is too short.');
        }
    }

    public function getName() 
    {
        return $this->name;
    }

    public function getPhone() 
    {
        return $this->phone;
    }

    public function getAddress() 
    {
        return $this->address;
    }

    public function getNotes() 
    {
        return $this->notes;
    }

    public static function printTable($sql)
    {
        $temp=  DataBaseActions::run_q($sql);
        while($row =  mysql_fetch_assoc($temp))
        {         
            $tmp='<tr class="content"><td>'.$row['name'].'</td><td>'.$row['phone'].'</td>
                  <td>'.$row['address'].'</td><td>'.$row['notes'].'</td>
                  <td class="try"><a href="index.php?mode=edit&id='.$row['contact_id'].'">Edit</a></td>
                  <td class="try"><a href="index.php?mode=delete&id='.$row['contact_id'].'">Delete</a></td></tr>';
            echo $tmp;
        }
    }
}

Database:

<?php
class DataBaseActions
{
    private $db_name='phonebook';
    private $db_username='Emanuela';
    private $db_host='localhost';

    public function connect()
    {
        mysql_connect($this->db_host, $this->db_username) or die('Error with the database.');
        mysql_select_db($this->db_name) or die ('Error with the database.');
    }

    public static function run_q($sql)
    {
        mysql_query('SET NAMES utf8');
        return mysql_query($sql);
    }

    public function insert($obj)
    {
        DataBaseActions::run_q('INSERT INTO contacts (name, address, phone, notes) 
            VALUES ("'.addslashes($obj->getName()).'", "'.addslashes($obj->getAddress()).'", 
                    "'.addslashes($obj->getPhone()).'", "'.addslashes($obj->getNotes()).'")');
    }

    public function update($obj, $id) 
    {
        DataBaseActions::run_q('UPDATE contacts SET name="'.$obj->getName().'", 
            address="'.$obj->getAddress().'", phone="'.$obj->getPhone().'", 
            notes="'.$obj->getNotes().'" WHERE contact_id='.$id);
    }

    public function delete($id)
    {
        DataBaseActions::run_q('DELETE FROM contacts WHERE contact_id='.$id);
    }

    public function sortTable($field, $mode)
    {
        $sql='SELECT * FROM contacts ORDER BY `'.$field.'`'.$mode;
        return $sql;
    }

    public function search($field, $value)
    {
        $keywords = explode(' ',$value);
        $query = 'SELECT * FROM contacts WHERE';
        foreach ($keywords as $key)
        {
            $query .= ' `'.$field.'` LIKE "%'.$key.'%" AND';
        }
        $query = rtrim($query,' AND');
        return $query;
    }
}

Pagination:

<?php
include 'DataBaseActions.php';

class Pagination 
{
    private $limit;
    private static $all=0;
    private $last_page;
    private $page;

    public function __construct($limit=5, $page=0) 
    {
        $this->limit = $limit;
        $this->page = $page;
        $this->last_page = ceil(Pagination::$all/$this->limit);
    }

    public function setAll($sql)
    {
        $temp=  DataBaseActions::run_q($sql);
        Pagination::$all=  mysql_num_rows($temp);
        $this->last_page = ceil(Pagination::$all/$this->limit);
    }

    public function setPage($page) 
    {
        $this->page = $page;
    }

    public function pageQuery($sql)
    {
        $newsql =$sql.' LIMIT '.$this->page*$this->limit.','.$this->limit;
        return $newsql;
    }

    public function printPages()
    {
        if($this->page>0)
        {

            echo '<a href="index.php?page='.($this->page).'">Previous</a>';
        }

        echo ' | ';
        for($i=0; $i<$this->last_page; $i++)
        {
            if($i==$this->page)
            {
                echo ($i+1);
            }
            else
            {
                echo '<a href="index.php?page='.($i+1).'">'.($i+1).' </a>';
            }
            echo ' | ';
        }

        if($this->page<($this->last_page-1))
        {
            echo '<a href="index.php?page='.($this->page+2).'"> Next</a><br>';
        }
    }   
}

And the index file which is the worst of all. There are so many ifs and the HTML and PHP code are so messy.

<?php
error_reporting(0);

include 'Contact.php';
include 'Pagination.php';
session_start();

$db=new DataBaseActions();
$db->connect();

if($_GET['mode']=='normal')
{
$flag=0;
$_SESSION['mode']='normal';
}

if($_GET['mode']=='delete')
{
$id = addslashes($_GET['id']);
$id = htmlspecialchars(stripslashes($id));
$id = mysql_escape_string($id);

$db->delete($id);
header('Location: index.php');
}

if ($_GET['mode'] == 'edit' && $_GET['id'] > 0) 
{
$id = (int) $_GET['id'];
$rs = DataBaseActions::run_q('SELECT * FROM contacts WHERE contact_id=' . $id);
$info = mysql_fetch_assoc($rs);
}

 if($_GET['mode']=='ASC')
{
$flag=1;
$field=$_GET['field'];
$mode='ASC';
$_SESSION['mode']='ASC';
$_SESSION['field']=$field;
 }

 if($_GET['mode']=='DESC')
{
$flag=1;
$field=$_GET['field'];
$mode='DESC';
$_SESSION['mode']='DESC';
$_SESSION['field']=$field;
}

 if($_POST['name']==1)
{
$s_name=$_POST['search_name'];
$flag=2;
$_SESSION['mode']='sname'; 
}

 if($_POST['phone']==1)
{
  $s_phone=$_POST['search_phone'];
  $flag=3;
  $_SESSION['mode']='sphone';
}

 if($_POST['address']==1)
{
  $s_address=$_POST['search_address'];
  $flag=4;
  $_SESSION['mode']='saddress';
}

 if($_POST['notes']==1)
{
  $s_notes=$_POST['search_notes'];
  $flag=5;
  $_SESSION['mode']='snotes';
}

 if($_POST['form_submit']==1)
{
$flag=0;
$name=$_POST['name'];
$phone=$_POST['phone'];
$address=$_POST['address'];
$notes=$_POST['notes'];
$_SESSION['mode']='normal';

try
{
    $a=new Contact($name, $phone, $address, $notes);

    $id=(int)$_POST['edit_value'];
    $temp=DataBaseActions::run_q('SELECT * FROM contacts WHERE phone='.$a->getPhone().' AND contact_id!='.$id);

    if(mysql_num_rows($temp)==0)
    {
        if($id>0)
        {
            $db->update($a, $id);
            echo 'Successful update.';
        }
        else
        {
            $db->insert($a);
            echo 'The contact was successfully added to the Phone Book!';
        }
   }
   else
   {
       echo 'This phone number is already in the Phone Book.';
   }

}
catch(Exception $exc)
{
    echo $exc->getMessage();
}
}
   ?>    

<!DOCTYPE html>
<html>
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
    <link rel="stylesheet" type="text/css" href="css/style.css" />
    <title>Phone Book</title>
</head>
<body>
    <center>
    <h3>Phone Book</h3><br />

    <form method="post" action="index.php">
        <table border="0">
        <tr><td>Name:</td><td> <input type="text" name="name" value="<?php echo $info['name'];?>"/></td></tr>
        <tr><td>Phone number:</td><td> <input type="text" name="phone" value="<?php echo $info['phone'];?>"/></td></tr>
        <tr><td>Address:</td><td> <input type="text" name="address" value="<?php echo $info['address'];?>"/></td></tr>
        <tr><td>Notes: </td><td><input type="textarea" name="notes" value="<?php echo $info['notes'];?>"/></td></tr>
        </table>
        <input type="hidden" name="form_submit" value="1" /><br />
        <input type="submit" value="Submit" /><br /><br />

        <?php
        if($_GET['mode']=='edit')
        {
            echo '<input type="hidden" name="edit_value" value="'.$_GET['id'].'" /><br />';
        }
        ?>

   </form>

    <table border="3" style="border-collapse:collapse; border-color: gray; padding: 2px;">

    <tr class="header">
    <td>Name</td>
    <td>Phone number</td>
    <td>Address</td>
    <td>Notes</td>
    <td></td>
    <td></td>
    </tr>

    <?php
    $p=new Pagination();

    if((int)$_GET['page']>0)
    {
        $p->setPage((int)$_GET['page']-1);
    }

    if($_SESSION['mode']=='normal')
    {
        $flag=0;
    }
    if($_SESSION['mode']=='ASC' || $_SESSION['mode']=='DESC')
    {
        $flag=1;
        $field=$_SESSION['field'];
        $mode=$_SESSION['mode'];
    }
    if($_SESSION['mode']=='sname')
    {
        $flag=2;
    }
    if($_SESSION['mode']=='sphone')
    {
        $flag=3;
    }
    if($_SESSION['mode']=='saddress')
    {
        $flag=4;
    }
    if($_SESSION['mode']=='snotes')
    {
        $flag=5;
    }

    switch($flag)
    {
        case 0:
            $sql='SELECT * FROM contacts';
            $p->setAll($sql);
            Contact::printTable($p->pageQuery($sql));
            break;
        case 1:
            echo'<a href="index.php?mode=normal"> Go back to unsorted  table<br></a>';
            $p->setAll($db->sortTable($field, $mode));
            Contact::printTable($p->pageQuery($db->sortTable($field, $mode)));
            break;
        case 2:
            echo'<a href="index.php?mode=normal">Go back to all contacts<br></a>';
            $p->setAll($db->search('name', $s_name));
            Contact::printTable($p->pageQuery($db->search('name', $s_name)));
            break;
        case 3:
            echo'<a href="index.php?mode=normal">Go back to all contacts<br></a>';
            $p->setAll($db->search('phone', $s_phone));
            Contact::printTable($p->pageQuery($db->search('phone', $s_phone)));
            break;
        case 4:
            echo'<a href="index.php?mode=normal">Go back to all contacts<br></a>';
            $p->setAll($db->search('address', $s_address));
            Contact::printTable($p->pageQuery($db->search('address', $s_address)));
            break;
        case 5:
            echo'<a href="index.php?mode=normal">Go back to all contacts<br></a>';
            $p->setAll($db->search('notes', $s_notes));
            Contact::printTable($p->pageQuery($db->search('notes', $s_notes)));
            break;
    }
    ?>

    <tr class="content">
    <td><form method="post" action="index.php"><input type="text" name="search_name">
            <input type="submit" value=">"><input type="hidden" name="name" value="1"></form></td>
    <td><form method="post" action="index.php"><input type="text" name="search_phone">
            <input type="submit" value=">"><input type="hidden" name="phone" value="1"></form></td>
    <td><form method="post" action="index.php"><input type="text" name="search_address">
            <input type="submit" value=">"><input type="hidden" name="address" value="1"></form></td>
    <td><form method="post" action="index.php"><input type="text" name="search_notes">
            <input type="submit" value=">"><input type="hidden" name="notes" value="1"></form></td>
    <td></td>
    <td></td>
    </tr>

    <tr class="content">
    <td><a href="index.php?mode=ASC&field=name">Sort ASC</a></td>
    <td><a href="index.php?mode=ASC&field=phone">Sort ASC</a></td>
    <td><a href="index.php?mode=ASC&field=address">Sort ASC</a></td>
    <td><a href="index.php?mode=ASC&field=notes">Sort ASC</a></td>
    <td></td>
    <td></td>
    </tr>

    <tr class="content">
    <td><a href="index.php?mode=DESC&field=name">Sort DESC</a></td>
    <td><a href="index.php?mode=DESC&field=phone">Sort DESC</a></td>
    <td><a href="index.php?mode=DESC&field=address">Sort DESC</a></td>
    <td><a href="index.php?mode=DESC&field=notes">Sort DESC</a></td>
    <td></td>
    <td></td>
    </tr>

    </table> 
    <br>

    <?php
    $p->printPages();
    ?>

    </div>
    </center>

</body>
</html>

Answer 1

Don't know if this is just poor question formatting, or your style, but please indent properly, this isn't really good:

class Contact
{
private $name;
private $phone ;
private $address;
private $notes ;
}

This would be better:

class Contact
{
    private $name;
    private $phone ;
    private $address;
    private $notes ;
}

I'd rewrite Contact::__construct as:

if (!$this->isValid($name, 3)) throw new Exception('The name is too short.');    
if (!$this->isValid($phone, 3)) throw new Exception('The phone number is too short.');

// at this point both checks have succeeded, no point in rechecking 
$this->name = addslashes(trim($name));
$this->phone = addslashes(trim($phone));
$this->address = addslashes(trim($address));
$this->notes = addslashes(trim($notes)); 

for brevity, readability and to not have redundant checks. Contact::isValid is not that expensive, still no point in calling it more times than necessary.

I'd advice against having HTML in your classes, as you do in Contact::printTable. Simplest solution would be to create an array of your database results, return the array and construct the table when it's absolutely necessary - at the script you actually show it.

Moving on to DataBaseActions, mysql_query('SET NAMES utf8'); is called every time DataBaseActions::run_q is called, and that's absolutely unnecessary. Not really an expensive database call, still redundant, you can safely move it into DataBaseActions::connect, it's a call that only needs be done once, just after you connect.

In general, avoid mysql_* functions, they are essentially obsolete, kept around only for legacy reasons. Their use is discouraged in the manual:

This extension is not recommended for writing new code. Instead, either the mysqli or PDO_MySQL extension should be used.

mysqli is a drop in replacement, all you need to do is add that extra i to all your functions (yes, it's that easy ;), but I'd strongly advice exploring PDO. You don't do any kind of validation on the stuff you throw at the database, your code is vulnerable to all sorts of trouble, the scariest one being SQL injection, and the simplest solution would be to use prepared statements. For example, how can you be certain $id is an integer when you call delete($id)?

In Pagination you also have some HTML, ideally you should move it out of the class, still given the class' nature, don't make it a top priority. As is, your class isn't reusable, if you want to have a different looking pagination somewhere you'd have to change the HTML in the class (and then your first pagination would look weird ;). Read up on separation of presentation and content.

Architecturally, there's a bit of a mess with DataBaseActions::run_q, as it's declared static when it's not really static, as it depends on DataBaseActions having been instantiated (for the database connection to be established). That's more PHP's fault than your own, the way mysql_* utilizes the global namespace always made me chuckle a bit. Now, since it's not really static, there's no point in declaring it as such, just make it a normal public function and feed your DataBaseActions object where it's needed. Read up on dependency injection and try to avoid static functions if they are not absolutely necessary. You could just pass the object as a parameter in Contact::__construct and Pagination::_construct, and replace DataBaseActions::run_q with $this->databaseActions->run_q.

Your index file is indeed messy. Start by breaking it up into smaller files, include as appropriate. And... good luck ;) Overall, your code is good for a starter, you're in a good path. If you significantly alter your code, don't forget to post another question here, there's always room for improvement, that's just the nature of code reviews (but don't over-engineer, done is better than perfect ;)

Answer 2

An important OOP principle is the Single Responsibility Principle: a class should have only one responsibility. Input validation and output (such as is handled by Contact::printTable) should be in separate classes. You can create a table view that will work with arbitrary data classes to handle output; the key is to use a combination of self-description on the part of data classes (a.k.a. models) and a limited amount of reflection (reflection generally isn't very efficient, so its use should be limited; you can use it for default implementations in parents to speed up development and override it in children for efficiency once you've got it working): data classes would include methods that return information (names, types &c.) about their fields. Read up on the other SOLID principles, the MVC architecture pattern and multi-tier architectures for more (links follow at the end).

DataBaseActions is the beginning of the data mapper pattern. Run with it: separate out anything specific to contacts into a child class. That way, you can use DataBaseActions as the basis for other database access classes.

Exception is a little too generic to instantiate and throw; better to use an exception of a type that's specific to the exceptional situation. Often, you can use (possibly extending) one of the SPL exceptions.

addslashes shouldn't be used to escape data destined for an SQL statement. If you use prepared statements, it's a moot point, but you otherwise should use the escape function provided by the database extension. You also shouldn't escape data before you're getting it ready to pass it on to another system, so don't use addslashes as you do in Contact::__construct, and the whole:

$id = addslashes($_GET['id']);
$id = htmlspecialchars(stripslashes($id));
$id = mysql_escape_string($id);

sequence in index.php is a combination of busy work (the addslashes/stripslashes calls) and inappropriate escaping (the call to htmlspecialchars). The SQL escaping should be left to the database access layer (DAL).

The HTML uses some presentational markup (e.g. <center>), and uses some elements non-semantically (e.g. <br>/ and the table used to format the form inputs). HTML is a document structuring language, not a formatting language. Use CSS for presentation. You can set the bottom margin of the <h3> element if you want extra space after it. Input label text should be placed in <label> elements (this is particularly important for screen readers and other aids to disabled users). You can then style labels and inputs to get the same presentation as you do with tables, or put the labels and inputs in some other semantically appropriate (such as a definition list) and style that.

Textareas are separate elements, rather than being a type of <input>. The default value is specified by the content of the element, rather than as the value of its value attribute.

To prevent HTML injection vulnerabilities (such as cross-site scripting (XSS)), data that shouldn't be allowed to contain HTML should be escaped using (e.g.) htmlspecialchars. The content of a <textarea> is one of the few exceptions, as it won't be interpreted as HTML.

<style type="text/css">
  label {
    float: left;
    min-width: 6em;
    margin-right: 0.5em;
  }
  input, textarea {
    display: block;
  }
</style>

<form method="post" action="...">
  <fieldset>
    <legend>Search</legend>
    <label for="name">Name:</label>
    <input name="name" id="name" value="<?php echo htmlspecialchars($info['name']) ?>"/>

    <label for="phone">Phone number:</label>
    <input name="phone" id="phone" value="<?php echo htmlspecialchars($info['phone']) ?>"/>

    <label for="address">Address:</label>
    <input name="address" id="address" value="<?php echo htmlspecialchars($info['address']) ?>"/>

    <label for="notes">Notes:</label>
    <textarea name="notes" id="notes"><?php echo $info['notes']; ?></textarea>

    <input type="submit" value="Submit" />
  </fieldset>
</form>

Instead of a canned form for contacts, you could create a form view that would work with arbitrary models.

If a variable or array index might not be set, use isset to test it. That way your code will work when error reporting is set to include E_NOTICE (which you should do on your development server to catch typos). You can also make use of isset to test for (e.g.) $_POST['name'] and do away with the "form_submit" input.

One big advantage to PDO (and mysqli in PHP 5.4 and greater) is they support the Traversable interface for database results, meaning you can loop over the results using foreach rather than relying on any method from the DB class API. This helps with decoupling database access and output.

# DB access (in production code, would be part of various methods)
try {
    $contactQuery = $db->prepare('SELECT name, phone, address, notes FROM contacts WHERE ...');
    $contacts = $contactQuery->execute(array(...));
    /* without PDO::FETCH_PROPS_LATE, the constructor will overwrite the 
     * property values fetched from the database. Note this shows a good
     * reason not to perform validation in Contact: though the constructor 
     * arguments result in an invalid contact, it's immediately corrected
     * after the constructor call.
     */
    $contactQuery->setFetchMode(PDO::FETCH_CLASS|PDO::FETCH_PROPS_LATE, 
                                'Contact', array(NULL, NULL));
    $fields = array();
    for ($i=0; $i < $contactQuery->columnCount(); ++$i) {
        $columnInfo = $contactQuery->getColumnMeta($i);
        $fields[] = $columnInfo['name'];
    }
} catch (PDOException $exc) {
    ...
}

$rows = $contacts;

/* Output. Note the only requirements are that:
 * + $fields is an array
 * + the elements of $fields can be cast to strings
 * + $rows is traversable 
 * + the elements of $rows are objects with properties named in $fields
 *
 * In particular, $rows could be an array, a PDOStatement, a mysqli_result
 * or something else entirely
 */
?>
<table>
  <thead><th><?php echo implode('</th><th>', array_map('ucfirst', $fields)) ?></th></thead>
  <tbody>
    <?php foreach ($rows as $i => $row) { ?>
      <tr>
        <?php foreach ($fields as $field) { ?>
          <td><?php echo $row->$field ?></td>
        <?php } ?>
      </tr>
    <?php } ?>
  </tbody>
</table>

Further Reading

• Understanding MVC in PHP
• Write your own PHP MVC Framework (Part 1)
• Model View Controller
• N-Tier Application Architecture (in brief)
• 10 orm patterns: components of a object-relational mapper