Code Is Not Units / Single Responsibility Principle
One of the first problems you run into when writing unit tests is how difficult it is to get code coverage. That's because methods are often very large. As a rule of thumb you usually don't want to go past 20 lines of code. In OpenCart some methods, especially in controllers, go well over 100 lines. The getForm() method generating the admin product form is over 750 lines.
We can begin addressing this issue by more closely adhering to the single responsibility principle. Let's look at the admin ControllerCatalogProduct class which contains the getForm() method. In the current OpenCart 2.0 master the class is a staggering 1400 lines.
The first problem we run into is that it has fundamentally different responsibilities: product lists (getList), product forms (getForm), and autocomplete. These should all be split into their own classes.
The next issue is the scope of getForm(). It generates the entire page with those 750 lines of code. Looking closer we see why: getForm() also has two responsibilities, adding products and editing them. These should be broken down into at least their own methods (or even better, their own classes). This way the code is not constantly having to detect whether the product is being added or edited by looking for post values.
By separating the code out and combining it with the superglobal method defaults RFC numerous code snippets like:
Code: Select all
// Images
if (isset($this->request->post['product_image'])) {
$product_images = $this->request->post['product_image'];
} elseif (isset($this->request->get['product_id'])) {
$product_images = $this->model_catalog_product->getProductImages($this->request->get['product_id']);
} else {
$product_images = array();
}
Code: Select all
public function edit() {
$product_id = $this->request->get('product_id', 0);
// ...
// Images
$product_images = $this->model_catalog_product->getProductImages($product_id);
}
public function create() {
//...
// Images
$product_images = $this->request->post('product_image', []);
}
Finally, we should split large code blocks such as language loading, errors, action buttons, URL building, and breadcrumbs into their own methods. This not only helps readability, it encourages smart integration with the new events system. When something like errors are their own page element it becomes more obvious that it's a place where developers may want to add or override values.
Cycolomatic Complexity
For anyone not familiar with it, cyclomatic complexity is a code metric which shows how complex code is. It finds this by looking at how deeply nested decision paths such as if, elseif, for, foreach, case, and while are. The more complex the code the more difficult it is to maintain.
The Software Engineering Institute rates cyclomatic complexity the following (source):
1-10 a simple program, without much risk
11-20 more complex, moderate risk
21-50 complex, high risk program
greater than 50 untestable program (very high risk)
Using PHPMetrics to analyze OpenCart's code it's clear there are some difficulties (see attached file for the full reports).
Admin Controllers
The cyclomatic complexity of certain classes exceeds 200. Controllers average 32 and the system library 23. Models average a more maintainable (but still too high) cyclomatic complexity of 17.
We can see where the complexity comes with the following code from the product model:
As a rule of thumb you shouldn't go more than 1 or (in a pinch) 2 levels deep with nesting. If you're going 3 or more it's time to strongly consider refactoring. Here ModelCatalogProduct::addProduct() is nested 5 levels deep. The code would benefit greatly from turning the options and many of the other subroutines into their own methods.
Improving Code
I highly recommend Rafael Dohms' talk Your Code Sucks, Let's Fix It. Don't let the inflammatory title throw you. It's an excellent lecture about improving code readability, maintainability, and ease of testing with many examples. I recently was able to refactor some old code using these methods with marked improvements:
The color represents the maintainability index of the code. It's calculated comparing cyclomatic complexity versus lines of code. Green is easy to maintain, yellow and orange/red harder. By further specializing classes, decreasing nesting, and adding more detailed docblocks the package maintainability index went from 80 to 103.
Here are OpenCart's admin controllers for comparison:
The largest dots represent a cyclomatic complexity over 200 while the smaller are around 10. The full HTML reports are available for download below.
There are also other issues which would benefit from discussion such as namespaces, a PSR compliant autoloader, Composer support, "skinny controllers, fat models", and so on. I'd also like to hear the opinions of the hobbyist programmers. The procedural code can be a headache for experienced programmers but it is arguably more accessible to store operators with limited coding experience than object oriented programming.
Short version: Improve OpenCart by further specializing classes, moving nested subroutines into their own methods, and reducing cyclomatic complexity.
