Phonebook - a small PHP project

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:

=$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=''.$row['name'].''.$row['phone'].'
                  '.$row['address'].''.$row['notes'].'
                  Edit
                  Delete';
            echo $tmp;
        }
    }
}

Database:

```
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');

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 ;)

StackExchange Code Review Q#13616, answer score: 6