Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm more of an IT guy (no CS course) with a strong and passionate relationship with Unix and I love KISS.

I'm writing an application to help my coworkers with their daily tasks. Every now and then I get request from coworkers to add functionality to help them, most of the time I will turn an Excel spreadsheet to a form based "module" in my application.

The application is built with PHP and runs on Apache, using MultiViews for "pretty-urls" and FallbackResource /index.php, the folder structure looks like

app/
    _views/
        _errors/
            401.php
            403.php
            404.php
        user/
            add.php
            get.php
    core/
        config.php
        user.php
    hr/
        department.php
    .settings.php
    bootstrap.php
lib/
    z/
        form.php
        route.php
        view.php
logs/
public/
    css/
    fonts/
    images/
    js/
    index.php
    user.php

app/bootstrap.php is prepended to every .php files in the public/ folder

<?php
// autoloader
set_include_path(implode(PATH_SEPARATOR, array(
    get_include_path(),
    dirname(__DIR__) . DIRECTORY_SEPARATOR .'lib',
    __DIR__,
)));
spl_autoload_extensions('.php');
spl_autoload_register();

// application user stuff
session_start();
if (isset($_SESSION['username'])) {
    $user = new core\user($_SESSION['username']);
    define('LOGGED', true);
}
else {
    $user = new core\user();
    define('LOGGED', false);
}

function http_error($error) {
    http_response_code($error);
    $view = new z\view('_errors/'. $error .'.php');

    die($view);
}

My controllers are in the public/ folder and look like this

<?php # /user.php ; controllers for /user(.*)
use z\form;
use z\route as main;
use z\view;

main::get('/user/add', function() use ($user) {
    if (!LOGGED)
            http_error(401);

    if (!$user->allowed('user-add'))
            http_error(403);

    $view = new view('user/add.php');

    die($view);
});

main::get('/user/%s', function($username) use ($user) {
    $person = new core\user($username);

    if ($person->notfound === true)
            http_error(404);

    if (!LOGGED)
            http_error(401);

    if (!$user->allowed('user-view'))
            http_error(403);

    $view = new view('user/get.php');
    $view->person = $person;

    if ($user->allowed('user-manage')) {
        $view->procedures = $person->procedures();
        $departments = hr\department::select();
        array_shift($departments);
        $view->departments = $departments;
    }

    die($view);
});

main::post('/user', function() use ($user) {
    if (!LOGGED)
            http_error(401);

    if (!$user->allowed('user-add'))
            http_error(403);

    $form = new form($_POST);
    if (core\user::insert(array(
        'value1' => $form->field1->value,
        'value2' => $form->field2->value,
        ...
    ))) {
        main::redirect('/user/'. $form->username->value);
    }
    else {
        $view = new view('user/add.php');
        $view->error = true;
        $view->message = 'Error message';

        die($view);
    }
});

...

http_error(404);

then in my view 'user/get.php' I will do something like

<?php $layout = 'html.php' ?>
<h1><?= $person->displayname ?></h1>
<p><?= $person->title ?></p>
<ul>
    <li>Office: <?= $person->phone_office ?></li>
    <li>Mobile: <?= $person->phone_mobile ?></li>
</ul>
<div class="tabs">
    <ul class="tabs-nav">
        <?php if ($user->allowed('user-manage-somestuff')): ?>
        <li><a href="#tab-1">Tab 1</a></li>
        <?php endif ?>

        <?php if ($user->allowed('user-manage-someotherstuff')): ?>
        <li><a href="#tab-2">Tab 2</a></li>
        <?php endif ?>

        <li><a href="#tab-3">Tab 3</a></li>
    </ul>

    <?php if ($user->allowed('user-manage-somestuff')): ?>
    <div id="tab-1"></div>
    <?php endif ?>

    <?php if ($user->allowed('user-manage-someotherstuff')): ?>
    <div id="tab-2"></div>
    <?php endif ?>

    <div id="tab-3"></div>
</div>

this is then wrapped in an 'html.php' layout by the view class.

The core\config class

<?php
namespace core;

class config
{
    private static
        $__settings = false;

    protected function __construct() {}

    public static function get($key) {
        if (!self::$__settings)
            self::$__settings = include '../.settings.php';

        $context = self::$__settings;
        $pieces = explode('.', $key);

        foreach ($pieces as $piece) {
            if (!is_array($context) || !array_key_exists($piece, $context))
                return false;

                $context = &$context[$piece];
            }

        return $context;
    }
}

The core\user class

<?php
namespace core;

use z\db;

class user
{
    public
        $displayname,
        $firstname,
        $lastname,
        $email,
        $phone_office,
        $phone_mobile,
        $notfound = true;

    protected
        $_roles = [];

    public function __construct($username = 'anonymous') {
        $this->displayname = 'Guest';
        $this->username = $username;
        if ($this->username != 'anonymous')
            $this->__init();
    }

    protected function __init() {
        $db = db::instance(config::get('db'));
        $stmt = $db->prepare("
            SELECT * FROM users WHERE username = :username");
        $stmt->execute(array(
            ':username' => $this->username
        ));
        $rslt = $stmt->fetch();
        if (!$rslt)
            return false;

        $this->firstname = $rslt['firstname'];
        $this->lastname = $rslt['lastname'];
        $this->displayname = $this->firstname .' '. $this->lastname;
        ...
    }

    public static function insert(array $user) {
        $db = db::instance(config::get('db'));
        $stmt = $db->prepare("
            INSERT INTO users(field1, ...)
            VALUES (:value1, ...)");

        return $stmt->execute(array(
            ':value1' => $user['field1'],
            ...
        ));
    }

    ...
}

The hr\department class look like

<?php
namespace hr;

use core\config;
use z\db;

class department
{
    public static function insert(array $department) {...}

    public static function select($id = -1) {...}

    public static function update(array $department) {...}

    public static function delete($id) {...}
}

As the application is getting bigger, I would like some advice on my code, can you see problems with the way I'm doing things?

Is this code "maintainable"? I wouldn't want my application to be removed if/when I leave and let my users in pain. My main concern here is if the guy who will replace me be able to understand and work with this code "easily". Note that I removed all code comments before posting but everything is documented in comments.

I'm also having a lot of difficulties trying to understand tests and how to write them, can someone explain to me how can I write tests for my application? and will I really benefit from writing/using them?

I spent a lot of time looking at PHP frameworks from Zend to Symphony, Kohana, CI, Laravel, Phalcon and many many others... I really liked some of the "micro-frameworks" like Slim and Lemonade but the file structure didn't make sense to me, why have such a complicated file structure and so many files/folders for such a simple thing?

Then I found a small PHP library web.php and took it's route, view and form class/functions as a starting point to build my application.

edit #1

I removed the path constant as I didn't really needed them.

edit #2

About the view class, I can use a different layout from inside a controller like this $view = new view('view.php', 'layout.php');

share|improve this question
    
There have been a number of comments on your Reddit thread that reference various things will be new to you. This resource is probably your best bet as a starting point: phptherightway.com –  bcmcfc Mar 4 at 8:52
    
I learned a lot from this website and can't thank enough it's author, it's clear, simple, I love it. But that was a few months ago. I think some of the practice I see are not really fit for the PHP "side" of a web application. –  teuna Mar 4 at 9:40
add comment

1 Answer 1

I wouldn't stress about tests if it's a smaller company, an internal application, and will not be distributed.

Generally a framework is a good idea. It gives you everything that you're asking for.

I'd recommend Yii for performance, but Cake because of its popularity, which makes it easily maintainable. So the next guy might already know it, and the documentation of the framework minimises your need to document. i.e. If he needs to do something he can just look on their site.

The simplest and most effective way forward would be to move it into Cake.

Was this application built proactively? Is it essential that you concern yourself with the future of this application?

After looking at the code:

There are some important structural problems in this application.

First off you need to read up on PHP best practices. You shouldn't use short tags.

You don't need the user and department classes. You should have a generic model class that will deal with these models, which can be extended if you need to do something special for a certain model. i.e. Object oriented with inheritance.

I see the $_POST goes straight into your form class. Does it get cleaned in there?

You are setting your layout in the view. What if you were doing something server side, such as browser detection, and then wanted to serve a different layout? You should set your layout where the logic is. In the controller.

Definitely move this into a common and accessible framework. Cake or Code Igniter. I think Yii maybe a bit too steep of a learning curve.

Also, because it is effectively business software, you will benefit from improved security within a framework.

share|improve this answer
    
The application wasn't build proactively and my only concern about it's future is my fellow coworkers using it (loving it and thanking me for it) everyday. The application did catch a couple managers attention so maybe it's something that will evolve in the company, at that point I'm not sure. $_POST is sanitazed by the form class yes, I can also do validation but for the sake of making this post shorter didn't included them. –  teuna Mar 2 at 21:02
    
There's nothing wrong with using PHP short tags (<?=). They are actively recommended in the community driven coding style guide PSR-1: php-fig.org/psr/psr-1 –  bcmcfc Mar 4 at 11:39
2  
That is not a recommendation. They are considered acceptable, but discouraged. What happens when you move files to a server with short tags off? Your code breaks, great! Use them, have fun ;) –  Darius Mar 4 at 15:27
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.