MVC WTF?

As a freelance developer, inheriting and working with code I did not originally write is common place. More often than not, code written using a known framework results in perfectly readable code, easy enough to work with and extend. Granted, every project is not written in Laravel 5, but the code that I inherit is workable.

When I recently inherited a complicated CodeIgniter site I was (initially) confident I’d have a good base to extend and work with. How wrong I was I? I could literally write a book about why the project I inherited was so bad. For the sake of brevity, I’ll stick to the main issues.

This post proves a single statement. Code written using a framework, is not necessarily “good”. It can end up painfully awful. In retrospect, I would have preferred to inherit a WordPress site. At least then my expectations are low to begin with 🙂

Base Controller?

When writing code, it’s good practice to use base classes, that are extended. For example, in CodeIgniter sites I typically run all controllers through a base “Front” or “Admin” controller. Similarly, when writing code using Laravel I commonly have classes like “BaseReposity” and “BaseTransformer”. Note the site in question, had 30 controllers. A typical controller was declared as follows:

class Profile_Over_View extends CI_Controller {
....

Huge issue – all 30 controllers extend a class located within the core system directory, meaning any changes made to the “CI_Controller” will be overwritten when updating. Not that you should be editing core classes in anyway, but still. For my case, the job specification required additional logic around reporting in each controller. If the previous developer had extended a custom base controller, the application would have been much more flexible.

Autoloading

CodeIgniter appreciates that certain libraries are used multiple times throughout an application and gives you a config file (aptly named “autoload.php”) to achieve this. So, if you require the url helper for every application request, auto load it. Do not do the following in every single controller:

$this->load->library(array('encrypt', 'form_validation', 'session'));
$this->load->helper(array('form', 'url'));

MVC, huh?

What follows is a typical controller – replicated across 30 files. The only differences between each controller is the name and the view it loads:

if ( ! defined('BASEPATH')) exit('No direct script access allowed');

class ProfileOverview extends CI_Controller {
	
	function Main()
	{
		parent::Controller();	
	}
	
	function index()
	{
		$this->load->library(array('encrypt', 'form_validation', 'session'));
		$this->load->helper(array('form', 'url'));

		$this->load->view('header');
		$this->load->view('profileview');
		$this->load->view('footer');
	}
	
}

Yep, every single controller looked like this. You may first look at this and think some magic is going on behind the scenes. Think again. Each controller loaded the same libraries and helpers and a view. Where is the logic to save resources, handle post requests etc. you may ask? Directly within the views of course!

Models, wtf?

Yup, the site made ZERO use of models. Nada. I’m not claiming every application requires the use of models, but for anything mildly complicated with business logic, models are a must. Unfortunately (for myself …), all logic is performed within views (see below). Need to repeat the same logic in another controller? No problem, copy and paste the 100 lines of code that create and validate a report, to the new controller (*&£*NT*^&N^Y*MO). For example, all the logic associated with say managing users is directly within the view. That is such bad practice.

Raw database queries are run for every database interaction. By having a model you can create a base model, that all your custom models extend. Of course you can abstract away common CRUD operations this way. There’s reason base models rock.

Views

In a well thought out application, views are minimal and contain only presentation logic. Typically, you may retrieve a list of usernames from the Model and output those names within the view in a list. Well, in this application EVERYTHING happened within the view. Need to interact with some user data? Well, you’d see the following code within the view:

if(isset($_POST['login']))
{	

	// Next lets check the username and password exist on the database
	$login_user_output = $this->db->query("SELECT * FROM registered_users WHERE 
		email_login = '{$_POST['loginname']}' AND password = '{$_POST['loginpassword']}' AND log_blocked = '1'"
);

I’m sure everyone notices the a few issues here immediately, namely and amongst other things:

• Interacting directly with the $_POST array. CodeIgniter for example allows you retrieve such input, automatically sanitized and XSS free.
• No validation of posted data
• Such logic should be in a model/library

Templating Logic

Each controller is essentially a copy of the next. Each controller contains the following three lines:

$this->load->view('header_profile');
$this->load->view('profileview');
$this->load->view('footer_profile');

Between each controller, a different view is loaded. After using CodeIgniter for roughly 10 minutes, you soon realize that this repetition gets boring. The majority of people will use a layout, sometimes even loaded automatically from a base view. For all my projects I used the excellent template library by Phil Sturgeon, that of course means I just load a single view, that extends a master layout.

To compound matters, the previous developer was just being silly. Each controller had a different header and footer. If we’re in the user controller, we’d be loading the user/header file. In the posts controller, we’d load the post-header view. This of course repeated for each controller. I’ve never once come across a system that even came close to warranting different header and footer views for every single page. Well, this system certainly didn’t need it either. Like most web applications, a main menu is visible. When a user navigates through the site the current menu item is highlighted in some meaningful manner. The previous developer had created separate views for each of these instances. So, the header_profile view had the “Profile” menu item set as active. The “header_user” had the “User” menu item active,. Absolute and complete insanity. The same applied to footers too. *** facepalm ***.

So in my case, I needed to make some minor design changes to the footer. Currently, I’d need to go and edit a lot of very similar footer files, instead of one.

Code Repetition

As the developer has used controllers to manually create views, there is a huge amount of code duplication (duplication immediately raises red flags for any sane developer). For example, views that require a user to be logged in (all views with the exception of a couple) have the following code at the top of each view file (disclaimer: not my code):

// 1.	Set the session to start
session_start();
// 2.	Clear all the session values
session_unset();
// 3.	Now bring in the sessions from the database
$sess = $this->db->query("SELECT arr FROM table WHERE user_id = '{$this->uri->segment(4)}' 
		AND category_id = '{$this->uri->segment(2)}' AND code_lc = '{strtolower(base64_decode($_COOKIE['user']))}'");
if ($sess->num_rows() > 0)
{
	// 4.	Refresh the Session variables
	$row = $sess->row(); 
	session_decode($row->arr);
}

if(!isset($_COOKIE['visit']) || $_COOKIE['visit'] == '')
{ 
	header('Location: /');
	exit;
}

// 39 other lines of code followed ...

Now, whilst that is far from great code (hell, CodeIgniter alone has lots of functionality to assist here) there is one major issue. If I wish to perform updates to this logic (which of course was part of the job specification for myself) I’m stuck unless I go into every view file and manually change the code. Normally, this would live in a model, or some sort of base controller – in ONE place.

Database Interaction

As mentioned, the entire system runs raw database queries. There is no concept of a model unfortunately. Here is how the developer has ran queries upon the database:

$check = $this->db->query("SELECT * FROM user WHERE email = '{$_POST['email']}'");

Firstly, the developer had turned off the following config keys: “global_xss_filtering” and “csrf_protection”. It is good practice to validate, escape and sanitize data before interacting with the database. Here, the developer is using post data directly submitted from a login form. Hell, you can get automatic escaping without any effort, as simple as:

$check = $this->db->query("SELECT * FROM user WHERE email =?", [$_POST['email']]);

I could continue as there is lot more. For instance, lack of any validation when performing CRUD operations, using a very out of date CodeIgniter version (site was developed mid 2014 and the site uses version 2.0.3 – from 2011!) etc.

I’d love to name and shame the freelancer (who has done a significant amount of work using CodeIgniter) but obviously won’t. With a little thought the code I inherited could have been easily extendable and much quicker to work with.

In retrospect, the system reminds me of a prototype style application. Maybe you had an idea and wrote some quick code to at least get an initial version working.

For myself, the biggest concern was the scope of the project. This system was used by a fairly high profile client to make some highly business critical decisions. Surely a system like that requires at least some evidence of thought before development begins?

Have you come across anything similar?