Splitting code, am I doing it right?

Question

I would like to have my code reviewed, I'm trying to get a general model I'm using for my stuff going and I don't want to keep using wrong or inefficient code if I can.

My file structure is like this:

In conf there's a file sitting called database.conf.php with the following content:

<?php
$this->host = "localhost";
$this->user = "snowclouds_LoL";
$this->password = "****";
$this->database = "snowclouds_LoL";
?>

In functions I have my functions included like this in functions.php

<?php
//Constructor class for calling all function scripts.
require("database.php");
require("security.php");
require("show.php");
require("user.php");
require("misc.php");
?>

This is my database class:

<?php
class Database {
private $host;
private $user;
private $password;
private $database;
private $link;

public function connect(){
    $this->link = mysqli_connect ($this->host, $this->user, $this->password) or die("Connection problem: " . mysqli_error());
    mysqli_select_db($this->link, $this->database);
}

public function resultquery($query){
    $result = mysqli_query($this->link, $query) or die(mysqli_error($this->link));
    return $result;
}

public function insertquery($query){
    if(mysqli_query($this->link, $query)){
        return true;
    }else{
        return mysqli_error($this->link);
    }
}

public function __construct(){
    if(!file_exists("conf/database.conf.php")){
        die("conf/database.conf.php file does not exist!");
    }else{
        include('conf/database.conf.php');
    }
    }
}
?>

Indenting is a little broke because pasting 4 spaces before every line is just time consuming :/

This is my index.php

$show->header();
$show->menu();
require("act.php");
$show->footer();
?>

This is my init.php

<?php
session_start();
ob_start();

require ("functions/functions.php");

//Check security for hackers.
$security = new Security();
$security->checkuri();

$user = new User();
$misc = new Misc();
$show = new Show();
?>

And in my act.php I write code like this:

if(isset($_GET['act']) & $_GET['act'] == "lostpass"){
    if(isset($_SESSION['username'])){
        $show->error("U bent al ingelogd met een bestaand account en u kunt uw wachtoowrd dus gewoon aanpassen op de aanpas pagina.");
    }else{
        include("view/lostpass.php");
    }
}

The files in view contain mostly html code with an occasional while for tables.

So how is this structure?

Is it good or unbelievably bad? (I want to stay away from stuff like Smarty because I want my own model for this kind of stuff and I'm still learning).

Extra stuff:

This is my Misc class:

<?php
class Misc{
    function check_email($email) {
      // First, we check that there's one @ symbol, 
      // and that the lengths are right.
      if (!ereg("^[^@]{1,64}@[^@]{1,255}$", $email)) {
        // Email invalid because wrong number of characters 
        // in one section or wrong number of @ symbols.
        return false;
      }
      // Split it into sections to make life easier
      $email_array = explode("@", $email);
      $local_array = explode(".", $email_array[0]);
      for ($i = 0; $i < sizeof($local_array); $i++) {
        if
    (!ereg("^(([A-Za-z0-9!#$%&'*+/=?^_`{|}~-][A-Za-z0-9!#$%&
    ?'*+/=?^_`{|}~\.-]{0,63})|(\"[^(\\|\")]{0,62}\"))$",
    $local_array[$i])) {
          return false;
        }
      }
      // Check if domain is IP. If not, 
      // it should be valid domain name
      if (!ereg("^\[?[0-9\.]+\]?$", $email_array[1])) {
        $domain_array = explode(".", $email_array[1]);
        if (sizeof($domain_array) < 2) {
            return false; // Not enough parts to domain
        }
        for ($i = 0; $i < sizeof($domain_array); $i++) {
          if
    (!ereg("^(([A-Za-z0-9][A-Za-z0-9-]{0,61}[A-Za-z0-9])|
    ?([A-Za-z0-9]+))$",
    $domain_array[$i])) {
            return false;
          }
        }
      }
      return true;
    }
}
?>

It's just for stuff that's pretty annoying to fully put into everything each time I need it.

Security:

<?php

/**
 * @author Jeffro
 * @copyright 2011
 */


    class Security{

        var $privatekey = "thisshouldgoinaconfigfileaswell";

        function mcryptexists()
        {
            if (function_exists("mcrypt_encrypt")){
                return true;
            }else{
                return false;
            }
        }

        function sslactive()
        {
            if ($_SERVER['HTTPS']) { 
                return true;    
            }else{
                return false;
            }
        }

        function safe_b64encode($string) {
            $data = base64_encode($string);
            $data = str_replace(array('+','/','='),array('-','_',''),$data);
            return $data;
        }

        function safe_b64decode($string) {
            $data = str_replace(array('-','_'),array('+','/'),$string);
            $mod4 = strlen($data) % 4;
            if ($mod4) {
                $data .= substr('====', $mod4);
            }
            return base64_decode($data);
        }

        function encode($value){ 
            if(!$value){return false;}
            $text = $value;
            $iv_size = mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB);
            $iv = mcrypt_create_iv($iv_size, MCRYPT_RAND);
            $crypttext = mcrypt_encrypt(MCRYPT_RIJNDAEL_256, $this->privatekey, $text, MCRYPT_MODE_ECB, $iv);
            return trim($this->safe_b64encode($crypttext)); 
        }

        function decode($value){
            if(!$value){return false;}
            $crypttext = $this->safe_b64decode($value); 
            $iv_size = mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_ECB);
            $iv = mcrypt_create_iv($iv_size, MCRYPT_RAND);
            $decrypttext = mcrypt_decrypt(MCRYPT_RIJNDAEL_256, $this->privatekey, $crypttext, MCRYPT_MODE_ECB, $iv);
            return trim($decrypttext);
        }

    }

    ?>

That's from my new project btw. (Plesk API system)

And here is my show class:

    <?php
    class show{
        function header(){
            include("view/header.php");
        }

        function menu(){
            include("view/menu.php");
        }

        function footer(){
            include("view/footer.php");
        }

        function error($message){
            echo $message;
            $this->footer();
            die();
        }
    }
    ?>


User Class: 

<?php
class User{
    private $database;
    function exists($user){
        $this->database->connect();
        if($result = $this->database->resultquery("SELECT * FROM users WHERE got_username='" . $user . "'")) {
            $count = mysqli_num_rows($result);
            if($count > 0){
                return true;
            }else{
                return false;
            }
        }
    }

    function isfriend($user, $friend){
        $this->database->connect();
        if($result = $this->database->resultquery("SELECT * FROM friends INNER JOIN users ON friends.user_id = users.user_id WHERE users.got_username='" . $user . "' AND friends.friend_id='" . $friend . "'")) {
            $count = mysqli_num_rows($result);
            if($count > 0){
                return true;
            }else{
                return false;
            }
        }
    }

    function addfriend($username, $friend){
        $this->database->connect();
        $user = $this->getuser($username);
        $result = $this->database->insertquery("INSERT INTO friends (user_id, friend_id) VALUES ('" . $user['user_id'] . "', '" . $friend . "')");
        return $result;
    }

    function delfriend($username, $friend){
        $this->database->connect();
        $user = $this->getuser($username);
        $result = $this->database->insertquery("DELETE FROM friends WHERE user_id = '" . $user['user_id'] . "' AND friend_id = '" . $friend . "'");
        return $result;
    }

    function updateuser($got_username, $eu_username, $us_username, $user_client, $play_ranked, $user_email, $user_password, $user_password_check, $comment, $ip, $date){
        $this->database->connect();
        $result = $this->database->insertquery("
        UPDATE users SET EU_username='$eu_username', US_username='$us_username', user_client='$user_client', plays_ranked='$play_ranked', comment='$comment', user_email='$user_email', user_password='" . sha1($user_password) . "', last_login='$date', last_ip='$ip'
        WHERE GoT_username='$got_username'");
        return $result; 
    }

    function updateusernopass($got_username, $eu_username, $us_username, $user_client, $play_ranked, $user_email, $comment, $ip, $date){
        $this->database->connect();
        $result = $this->database->insertquery("
        UPDATE users SET EU_username='$eu_username', US_username='$us_username', user_client='$user_client', plays_ranked='$play_ranked', comment='$comment', user_email='$user_email', last_login='$date', last_ip='$ip'
        WHERE GoT_username='$got_username'");
        return $result; 
    }

    function register($got_username, $eu_username, $us_username, $user_client, $play_ranked, $user_email, $user_password, $user_password_check, $comment, $ip, $date){
        $result = $this->database->insertquery("
        INSERT INTO users (GoT_username, EU_username, US_username, user_client, plays_ranked, comment, user_email, user_password, reg_date, last_login, last_ip)
        VALUES ('$got_username', '$eu_username', '$us_username', '$user_client', '$play_ranked', '$comment', '$user_email', '". sha1($user_password) ."', '$date', '$date', '$ip')");
        return $result; 
    }

    function getusers(){
        $this->database->connect();
        $query = "SELECT * FROM users ORDER BY user_id DESC";
        $result = $this->database->resultquery($query);
        $array = array();
        while($value = mysqli_fetch_array($result))
        {
            $array[] = $value;
        }
        return $array;
    }

    function inserthash($email, $hash){
        $this->database->connect();
        $query = "UPDATE users SET user_hash='$hash' WHERE user_email='$email'";
        $result = $this->database->insertquery($query);
        return $result;
    }

    function updatepassword($hash, $password){
        $this->database->connect();
        $password =  sha1($password);
        $query = "UPDATE users SET user_password='$password' WHERE user_hash='$hash'";
        $this->database->insertquery("UPDATE users SET user_hash='' WHERE user_hash='$hash'");
        $result = $this->database->insertquery($query);
        return $result;
    }

    function existsbyemail($email){
        $this->database->connect();
        $query = "SELECT * FROM users WHERE user_email = '$email'";
        $result = $this->database->resultquery($query);
        $count = mysqli_num_rows($result);
            if($count > 0){
                return true;
            }else{
                return false;
            }
    }

    function getuserbyemail($email){
        $this->database->connect();
        $query = "SELECT * FROM users WHERE user_email = '$email'";
        $result = $this->database->resultquery($query);
        $array = mysqli_fetch_array($result);
        return $array;
    }

    function getuser($username){
        $this->database->connect();
        $query = "SELECT * FROM users WHERE GoT_username = '$username'";
        $result = $this->database->resultquery($query);
        $array = mysqli_fetch_array($result);
        return $array;
    }

    function login($username, $password){
        $this->database->connect();
        $query = "SELECT * FROM users WHERE GoT_username = '$username' AND user_password = '". sha1($password) . "'";
        $result = $this->database->resultquery($query);
        $count = mysqli_num_rows($result);
        if($count > 0){
            return true;
        }else{
            return false;
        }
    }

    function logout(){
        unset($_SESSION['GoT_username']);
        unset($_SESSION['users_password']);
    }

    public function __construct(){
        $this->database = new Database();
    }
}
?>

Answer 1

The only thing I see so far is that you have high Coupling between the Database class and the location of the config file. What you should do is either:

• Create a config class that handles config parsing and setting
• Or, at minimum, load the config file somewhere else

In both cases you will need to pass the parts that matter to the Database constructor. This reduces Coupling which, in turn, reduces the amount of rewrite needed during refactoring.

There is little more I can offer except questions. For example, what is Security doing? Is there only the one function? What else is declared in it? The same goes for Misc. Both of these classes sound like they don't really belong and are classes for the sake of being classes.

EDIT

... the config class sounds interesting. Do you have any good places where I should start looking for a decent way?

The Zend_Config documentation, part of the Zend Framework, would probably be a good place to see how people are using something like this.

This is my Misc class:

You could keep this I suppose, but Misc is a poor name for it. Calling some thing Misc is just asking for disorganization. If you really want this to be a function, and you can support PHP 5.3, I would wrap this in a namespace and treat the namespace similar to what most people would call a module.

Example:

<?php
namespace Utilities
{
    function validateEmail( $email )
    {
        // Code
    }
}

However, you should really consider refreshing yourself on the PHP documentation, specifically the newer stuff that's been added since 4.0. A perfect example of this is the use of the Filter extension. In your case, FILTER_VALIDATE_EMAIL will do wonders.

Security:

This class, I won't really comment on because I'm not sure the use case or need. I'll assume it is necessary as is.

Similar to Database you have high Coupling by putting the private key directly into the class. Depending on the needs of the key, it could exist in a config file and be treated just like the Database config attributes. On initialization, you would pass the config var for passkey into Security::__constructor.

Doing so means that you could have a different passkey per user instead of a single passkey. Expanding this to a public+private key system in the future would also be easy (just add another parameter).

User:

This class is also plagued with high Coupling because of the self loading of Database. There are a few ways to handle dependency injection and I'm not going to sway you either way on this matter. However, I'd suggest you spend some time looking through the many articles your favorite search engine will provide (or Bing! and decide...lol).

I would also keep from naming functions in all lower case. It is common for people to name functions either in CamelCase or underlines _ between words. Not doing so can impact readability.

Aside form that, this class is the first I've seen you provide that is acting as a Model (thinking in terms of MVC). That's a good thing, so nice job. I would expand your understanding of MVC if this is the path you are headed for so that you don't fall into many of the common pitfalls.

Answer 2

That's a good split to start off with, although I strongly encourage you to place your database conf file outside the public html area on your server.