Post by rph » Mon Sep 22, 2014 7:15 am

I've been writing some unit tests for OpenCart recently as a way to update my skills from SimpleTest (which is all but dead) to PHPUnit. It's reinforced a need for code improvements that would benefit OpenCart and the developer community. This is absolutely not meant to slander anyone. The only goal is improving OpenCart.

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.

Image

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();
}
 
can be reduced down to a more managable:

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', []);
}
 
Even better, we don't have to worry about testing coverage anymore. The code is already covered by unit tests for ModelCatalogProduct::getProductImage() and Request::post()!

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).

Image
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:

Image

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:

Image

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:

Image

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.

-Ryan


rph
Expert Member

Posts

Joined
Fri Jan 08, 2010 5:05 am
Location - Lincoln, Nebraska

Post by Malaiac » Mon Sep 22, 2014 3:40 pm

Phew, nice work !
Separating add/edit responsabilities sure would be an improvement.
Creating multiple classes for the same "admin/catalog/item" management seems more complex than the current structure (and could break vqmod plugins).

Just viewed the " Your Code Sucks, Let's Fix It." video, very instructing.

New member

Posts

Joined
Thu Jan 07, 2010 12:06 am

Post by Johnathan » Mon Sep 22, 2014 11:21 pm

Even if none of this gets implemented, thanks for the detailed post, Ryan. It's very instructive.

Image
Image Image Image Image


User avatar
Global Moderator

Posts

Joined
Fri Dec 18, 2009 3:08 am


Post by rph » Wed Sep 24, 2014 12:02 am

Thanks Johnathan. I know it's late in the OC 2.0 development process to start doing major refactoring. We could always target specific areas for refactoring then push out the changes in minor releases, i.e. refactor admin product controllers in 2.1, library classes in 2.2 and so on. It would also be a great time to write the unit tests for them.

-Ryan


rph
Expert Member

Posts

Joined
Fri Jan 08, 2010 5:05 am
Location - Lincoln, Nebraska

Post by rph » Wed Sep 24, 2014 12:39 am

Malaiac wrote:Creating multiple classes for the same "admin/catalog/item" management seems more complex than the current structure (and could break vqmod plugins).
Most mods are going to be broken for 2.0 regardless but I'm okay with pushing them to their own methods. The important part is addressing the dozens of lines of duplicate logic.
Just viewed the " Your Code Sucks, Let's Fix It." video, very instructing.
It's a bit difficult to implement at first (I still have a few "else" statements floating around in my code even after months of using it) but once the style "clicks" it's a huge improvement.

One thing I didn't get to talk about is adding PHP docblocks. I used to be in the "good code is self-documenting" school (I still am to an extent) but adding docblocks would be a good way of finally getting OpenCart's code documented and creating uniform standards.

You could even use it to stub out code so developers could begin implementing it in their own dev code or contribute work to complete it:

Code: Select all

/**
 * This code will return the product name for corresponding id when completed.
 *
 * @param int $product_id
 * @return string
 * @todo Write method
 */
public function apiGetProductName($product_id) {}

/**
 * This code will return the product quantity for corresponding id when completed.
 *
 * @param int $product_id
 * @return int
 * @todo Write method
 */
public function apiGetProductQuantity($product_id) {}
 

-Ryan


rph
Expert Member

Posts

Joined
Fri Jan 08, 2010 5:05 am
Location - Lincoln, Nebraska

Post by Daniel » Sun Sep 28, 2014 7:38 pm

interesting, why don't upload some demo code of how you would change opencart. maybe just a form and list controller.

OpenCart®
Project Owner & Developer.


User avatar
Administrator

Posts

Joined
Fri Nov 03, 2006 6:57 pm

Post by rph » Mon Sep 29, 2014 1:45 am

I'll try to throw something together when I've got a bit of time. Just doing a real quick and dirty refactor on something simple like autocomplete, we can reduce it from:

Code: Select all

public function autocomplete() {
    $json = array();

    if (isset($this->request->get['filter_name']) || isset($this->request->get['filter_model'])) {
        $this->load->model('catalog/product');
        $this->load->model('catalog/option');

        if (isset($this->request->get['filter_name'])) {
            $filter_name = $this->request->get['filter_name'];
        } else {
            $filter_name = '';
        }

        if (isset($this->request->get['filter_model'])) {
            $filter_model = $this->request->get['filter_model'];
        } else {
            $filter_model = '';
        }

        if (isset($this->request->get['limit'])) {
            $limit = $this->request->get['limit'];
        } else {
            $limit = 5;
        }

        $filter_data = array(
            'filter_name'  => $filter_name,
            'filter_model' => $filter_model,
            'start'        => 0,
            'limit'        => $limit
        );

        $results = $this->model_catalog_product->getProducts($filter_data);

        foreach ($results as $result) {
            $option_data = array();

            $product_options = $this->model_catalog_product->getProductOptions($result['product_id']);

            foreach ($product_options as $product_option) {
                $option_info = $this->model_catalog_option->getOption($product_option['option_id']);

                if ($option_info) {
                    $product_option_value_data = array();

                    foreach ($product_option['product_option_value'] as $product_option_value) {
                        $option_value_info = $this->model_catalog_option->getOptionValue($product_option_value['option_value_id']);

                        if ($option_value_info) {
                            $product_option_value_data[] = array(
                                'product_option_value_id' => $product_option_value['product_option_value_id'],
                                'option_value_id'         => $product_option_value['option_value_id'],
                                'name'                    => $option_value_info['name'],
                                'price'                   => (float)$product_option_value['price'] ? $this->currency->format($product_option_value['price'], $this->config->get('config_currency')) : false,
                                'price_prefix'            => $product_option_value['price_prefix']
                            );
                        }
                    }

                    $option_data[] = array(
                        'product_option_id'    => $product_option['product_option_id'],
                        'product_option_value' => $product_option_value_data,
                        'option_id'            => $product_option['option_id'],
                        'name'                 => $option_info['name'],
                        'type'                 => $option_info['type'],
                        'value'                => $product_option['value'],
                        'required'             => $product_option['required']
                    );
                }
            }

            $json[] = array(
                'product_id' => $result['product_id'],
                'name'       => strip_tags(html_entity_decode($result['name'], ENT_QUOTES, 'UTF-8')),
                'model'      => $result['model'],
                'option'     => $option_data,
                'price'      => $result['price']
            );
        }
    }

    $this->response->addHeader('Content-Type: application/json');
    $this->response->setOutput(json_encode($json));
}
down to:

Code: Select all

class ControllerCatalogProduct extends Controller {
    // ...
    public function autocomplete() {
        $this->load->model('catalog/product');

        $filter = array(
            'filter_name'  => $this->request->get('filter_name'),
            'filter_model' => $this->request->get('filter_model'),
            'start'        => 0,
            'limit'        => $this->request->get('limit', 5)
        );

        $json = $this->model_catalog_product->getProductsAutocomplete($filter);

        $this->response->addHeader('Content-Type: application/json');
        $this->response->setOutput(json_encode($json));
    }
}

Code: Select all

class ModelCatalogProduct extends Model {
    // ...
    public function getProductsAutocomplete($filter) {
        $products = array();

        $results = $this->getProducts($filter);

        foreach ($results as $result) {
            $products[] = array(
                'product_id' => $result['product_id'],
                'name'       => strip_tags(html_entity_decode($result['name'], ENT_QUOTES, 'UTF-8')),
                'model'      => $result['model'],
                'option'     => $this->getProductOptions($result['product_id']),
                'price'      => $result['price']
            );
        }

        return $products;
    }
}
A lot of the autocomplete method was duplicating functionality that already existed in the product model. Putting together a little model method to bring it all together reduced the lines of code from 82 to 29 (65% smaller) and decreased cyclomatic complexity from 10 to 1. Adopting a more robust "fat model, skinny controller" design will reduce a lot of code like that.

-Ryan


rph
Expert Member

Posts

Joined
Fri Jan 08, 2010 5:05 am
Location - Lincoln, Nebraska

Post by dfumagalli » Mon Sep 29, 2014 5:09 pm

I'd like to add a couple of considerations.

When developing my extensions, I also use my own PHP Unit tests and indeed some OpenCart functions are hard to stick into a test.

I agree on most of what's written here but I'd also caution against going "all out" in that direction.

One of the best OpenCart features is that even a barely trained monkey can achieve effective and quick results with modding it. The other is OpenCart speed.
Both of them made OpenCart the premiere choice for small scale developers (see how it's popular in India!).

It'd be cool if after those optimizations, OpenCart still retained those strong points.

So, OK at reducing those tedious and repeated "read request, see if there is a default, else read from this and that" sequence.

But please don't forget PHP is not Java, so I would not fret over creating a trillion tiny classes, sub-sub-sub factories, each with five lines of code. "Because then it looks good on cyclomatic complexity".

A getForm() looks ugly but it's easy as hell to customize. Actually, it's because OpenCart is coded in such a linear way that it's easy to understand and vQMods can be so easily implemented.

If we start putting sub-sub files all over the places, PSR, Composer, OpenCart starts looking like that fat, slow dog called Magento or like that other slow dog called Symfony. If OpenCart complexity starts looking like them, then we lose the OpenCart strong points I listed above.

To me, complexity comes from having to check 20 files and their side effects, just to follow a single file logic.

A current OpenCart controller looks ugly and bloated, yes. But it's all there. Also, it's self contained, I had to make controller specific mods that only affect a certain controller "rules" that would be very hard if controllers depended from a central management. I then would have to do what's even more horrid than code bloat: go in the base classes used by all controllers and go place terrible "if calling controller == Name => do specific stuff".
Unless, of course, we totally refactor OpenCart to have super-overrideable local features, requiring other classes and more classes.

So, please, optimize where it really cuts off boilerplate but don't twist its simple and fast architecture into a Java clone. Then, really we'll have to produce (and modders read and understand) wagons of documentation just to understand what the 25 interwined classes in each method do.

Heavy OpenCart Customizations. Current project in progress: fleurworld.com


Active Member

Posts

Joined
Tue Aug 20, 2013 3:34 am

Post by rph » Tue Sep 30, 2014 7:37 am

I agree, a balance is needed. It shouldn't be all about changing numbers on a static analysis report. But at the same time cyclomatic complexity does represent a real hurdle in understanding and maintaining code. Have you ever tried to work on the Cart class or the admin order editor? It's difficult. That level of complexity also makes moding a challenge. Code responsibilities are hard to locate and vQmoding nested code is error prone.

I've started preliminary work on refactoring the product form and list. It's easy to see why there's such a clamoring to get event hooks into the controllers. Tons of model and service level work is occurring there. Want to add page text? You'll need to hook into the controller because there isn't a dedicated language model. Want to check for errors? You'll have to hook into the controller because there isn't a validation service. Want to modify certain product data? You'll have to hook into the controller because that's where a huge amount of the product data processing occurs instead of the model.

-Ryan


rph
Expert Member

Posts

Joined
Fri Jan 08, 2010 5:05 am
Location - Lincoln, Nebraska

Post by dfumagalli » Wed Oct 01, 2014 4:23 pm

rph wrote:I agree, a balance is needed. It shouldn't be all about changing numbers on a static analysis report. But at the same time cyclomatic complexity does represent a real hurdle in understanding and maintaining code. Have you ever tried to work on the Cart class or the admin order editor? It's difficult. That level of complexity also makes moding a challenge. Code responsibilities are hard to locate and vQmoding nested code is error prone.
I have had to deeply customize both, both in their default setup and once I even had to customize a "one page Ajax checkout" add on replacing the checkout. I think those are another dimension to cyclomatic complexity, as it can be seen those "big boys" components were developed over the years and code "stratified", some times even using different coding approaches compared the rest of OpenCart.

What I have found to be just harsh, was having to replace 216 OpenCart different code locations just to add *1* field that would be managed starting from the registration page address and up to including appearing in the checkout.
Plus, code was inconsistent, I could not just do a mass vQMod replace, since in some PHP files the same field is put in a "$data" array blob, in others it's managed as single field, in others it's in another named array blob and so on.

Those, in my opinion, would be the areas to refactor, that is how to extend OpenCart in a serious way (if adding a field may be called "serious extension") without pulling hairs.

rph wrote: I've started preliminary work on refactoring the product form and list. It's easy to see why there's such a clamoring to get event hooks into the controllers. Tons of model and service level work is occurring there. Want to add page text? You'll need to hook into the controller because there isn't a dedicated language model. Want to check for errors? You'll have to hook into the controller because there isn't a validation service. Want to modify certain product data? You'll have to hook into the controller because that's where a huge amount of the product data processing occurs instead of the model.
Imo the event model is a good starting point but it needs a big extension.
Whoever has coded WordPress plugins knows how good is to be able and setup filters and other "strong control" constructs. Otherwise we solve some pretty trivial tasks (like, adding a translation text with an OnXXX event) but the big meat (changing how a controller behaves in a non trivial way) still needs a O/vQMod.

A mild refactoring would help so much already. Imagine NOT splitting a large controller / model into 5-6 files but just splitting those 300+ lines of code functions in smaller blocks. It'd be finally viable to use Override Engine on a larger scale and just inherit & extend stuff a la good old OOP way.

Heavy OpenCart Customizations. Current project in progress: fleurworld.com


Active Member

Posts

Joined
Tue Aug 20, 2013 3:34 am

Post by Dhaupin » Thu Oct 02, 2014 1:37 am

Awesome writeup rph. Agree with both sides here, lean towards less cylexity, but not so much that everything is "micro".

This isnt really related to unit testing per say, but the amount of extra whitespace (spaces/tabs) in code is sometimes staggering. If comment blocks for doc/Dox are adopted, folks should make an initiative to clean those extra whitespace while theyre in the area adding comments. 2 birds.

https://creadev.org | support@creadev.org - Opencart Extensions, Integrations, & Development. Made in the USA.


User avatar
Active Member

Posts

Joined
Tue May 13, 2014 3:45 am
Location - PA

Post by rph » Sat Oct 04, 2014 7:34 am

I cleaned up a ton of the 2.0 code a few months back. I don't know what it looks like now.

I'm going to hold off on refactoring since 2.0 is already out. Probably not a good idea to shake up the code when so many devs are trying to learn it.

-Ryan


rph
Expert Member

Posts

Joined
Fri Jan 08, 2010 5:05 am
Location - Lincoln, Nebraska
Who is online

Users browsing this forum: sandybra and 6 guests