Model-View-Controller-Service shared flat expenses interface

Question

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.

• Mike Brant: You need to place the code you wish to have reviewed in your post.
• bumperbox: 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.

Answer 1

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.