1
\$\begingroup\$

I'm new to php and OOP, and for learning I started writing an application that would help me and my flatmates to manage our expenses and debts to one another.

The full source is available at https://bitbucket.org/nicocool84/coloc-web/src. I tried to use the MVC(S) architecture without using a framework.

The project tree is as follows:

.
├── controllers
│   ├── DebtController.php
│   ├── DefaultController.php
│   └── UserController.php
├── models
│   ├── DebtList.php
│   ├── Debt.php
│   └── User.php
├── services
│   ├── Database.php
│   ├── DebtService.php
│   ├── RedirectService.php
│   └── UserService.php
├── views
│   ├── footer.php
│   ├── header_logged.php
│   ├── header_unlogged.php
│   ├── home_unlogged.php
│   ├── html_header.php
│   ├── jumbotron.php
│   └── list_debts.php
└── web
    ├── index.php
    └── .htaccess

.htaccess redirects all requests to index.php which decides which controller and which action to call based on the URI: http://mysite.my/controller/action.

To handle sessions, when a user is authenticated, I store a User instance with his details in $_SESSION. The related methods are stored in UserService, for instance getCurrentName. Is this a good practice? Should I create a SessionService? Should I do this completely differently?

Is creating a DebtList class to compute who owes what making any sense?

I know the HTML/CSS part is quite ugly, and I'm open to advice there too.

Thanks to anyone who will take the time to review it all.

\$\endgroup\$
  • 1
    \$\begingroup\$ You need to place the code you wish to have reviewed in your post. \$\endgroup\$ – Mike Brant Oct 17 '16 at 2:19
  • \$\begingroup\$ Code reviews work best if you just post a small bit of code, reading through someones entire project is quite time consuming. for example you could start off by posting the DebtController. \$\endgroup\$ – bumperbox Oct 17 '16 at 8:23
1
\$\begingroup\$

Here is a start

I opened your code in php storm and it says Non-static method Database::getPdo() should not be called statically

Where you have code like this

public function deleteAction()
{
    $id = $_REQUEST['debt_id'];
    if (UserService::isLogged()) {
        if (DebtService::getPaidByFromId($id) == UserService::getCurrentUserId() or UserService::isAdmin()) {
            DebtService::deleteById($id);
            return DebtController::listAction();
        } else {
            RedirectService::goHome();
        }
    } else {
        RedirectService::goHome();
    }
}

I prefer to have a guard clause at the top, so you can easily see the outcome of the if/else without having to scroll to the end of the code

public function deleteAction()
{
    if (!UserService::isLogged()) {
        RedirectService::goHome();
    }

    // TODO what if debt_id doesn't exist, always check isset()
    $id = $_REQUEST['debt_id'];

    if (DebtService::getPaidByFromId($id) == UserService::getCurrentUserId() or UserService::isAdmin()) {
        DebtService::deleteById($id);
        return DebtController::listAction();
    } else {
        RedirectService::goHome();
    }        
}

Instead of this

$includes = array("controllers", "services", "models");

foreach ($includes as $dir) {
    foreach (glob("../" . $dir . "/*.php") as $file) {
        require $file;
    }
}

you might want to look at autoloading, it's not very hard to do http://php.net/manual/en/function.spl-autoload-register.php

In your templates you are doing

<?php echo $debt->getDescription(); ?>

Unless you know for sure there is no html or user editable content, you should really html escape it

<?php echo htmlentities($debt->getDescription()); ?>

Database service, take the config out of the service, and store it in a config file. otherwise you need to edit the code when you deploy to a different server.

Would a join work better here

// DebtService::getDebtsForMonth

SELECT date, amount, name AS description, paid_by, id, user_id
            FROM debts 
JOIN debtors ON debts.id=debtors.debt_id
WHERE MONTH(date) = ? and YEAR(date) = ? ORDER BY date

You may need to use exit; here. setting the location doesn't stop the code from running

public static function goToURI($uri_suffix)
{
    header("Location:" . uri_prefix . $uri_suffix);
    exit; 
}

It is not essential but I like to keep all my defines in uppercase

define('URI_PREFIX', "/coloc-web");

That should be enough to get your going for now.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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