Splitting code, am I doing it right?

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:

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

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

This is my database class:

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

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 unbelievab

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.

StackExchange Code Review Q#1247, answer score: 5