In world of programming and software development everyone is expected to deliver high quality code within a short period of time. One of the software development goals is to ensure that changes we made to a software application are thoroughly tested.

In the Agile world every single person plays an important role by defining requirements and expectations at an early stage of a development cycle as well as all iterations until product is released. Whether he or she is a Product Owner, Scrum Master, Quality Engineer or Software Engineer, everyone is a part of an ecosystem and important part of a Team.

You probably have the same question as I do: “What the hell am I talking about?”. 
The answer is simple: “Everyone should prove that the system works as expected”.

There are 2 ways that I know:

• The first one is to manually demonstrate working functionality each time you or your teammate made a change in code. 
However, it becomes a big problem in case you have such a big platform as Magento. Even if a change is a single line of code.
• The second option is to automate your efforts by various tests (e.g. Unit, Integration or Functional).

There is a third option where you pay somebody else to perform all dirty work instead of you. In this case you should be a billionaire like Steve Jobs or Bill Gates. Otherwise, fill free to continue reading, my dear Reader.

In this post I want to share my experience on how to refactor code in a safe way by using the Test Driven Development approach. Since most of my time I work with Magento 2 projects, all code that you will see in this post is going to be based on Magento 2.

What is Test Driven Development?

Before we start let’s be sure we are on the same page about the Test Driven Development (TDD).

Here are few links for you to get familiar with TDD. Guidelines for Test-Driven Development from Jeffrey Palermo and Introduction to Test Driven Development (TDD). In addition to this, Kent Beck provides a definition of TDD in his “Test Driven Development by Example” book. This book is a “must have” to read for every developer. Also, please check the “Refactoring, improving the design of existing code” book by Martin Fowler. You will learn a process of refactoring with great examples. By the way, this is one of my favourite books. And finally, please check “Clean Code: A Handbook of Agile Software Craftsmanship” book by Robert C. Martin.

Test Driven Development is a software development process which helps teams to build loosely coupled and highly cohesive code with low defect rate. One of the rules defined for TDD says that a developer should only change code when it is covered with automated tests. It means that automated tests should be created and executed first, and more important, you first should have an automated test which fails. Having a test that fails helps you quickly make it pass by creating necessary code. Once the test passes, you need to refactor source code to eliminate all code duplication which could appear when you were making the test pass. This is the main rule or TDD mantra: “Red, Green, Refactor”.

Pic. 1. TDD – Red, Green, Refactor steps

There are many advantages for using TDD and here are some of them:

• When you think about your code as a client, application design becomes more solid
• You definitely have tests in place
• Software program is ready to go live once all tests successfully pass

Magento 2 Tests

There are 7 different types of tests in the Magento 2 project. Check @alant99 blog post about Magento 2 Testing. Mostly all tests (except Unit Tests) are located under the corresponding folder in the < magento2root >/dev/tests directory. Starting from 0.74.0-beta1 release Magento team moved all Unit Tests to module directories.

Pic. 2. Magento 2 tests directory

In order to perform small and fast “Red, Green, Refactor” iterations, we are going to work with Unit Tests.

Unit Tests

Unit Test is a test which helps to verify a single class or a method. A class or a method under the test should be isolated from all external dependencies to other classes and libraries. Usually, in order to create test you need to download and configure PHPUnit framework. More information on writing tests you may find at PHPUnit Documentation space.

You should be aware that Magento 2 project stores Unit Tests phpunit.xml.dist configuration file under Magento2Root >/dev/tests/unit directory. Unit Tests in Magento 2 are located in corresponding Modules  Magento2Root >/app/code/< ModuleName >/Test .

Let’s Get This Party Started…

Searching for some issues on the Magento 2 GitHub, I’ve found an interesting comment from user ihor-sviziev. He mentioned that existing implementation of Magento\Sales\Model\Order\Payment class is overcomplicated. Refactoring of this class might be a great example of using TDD in practice.

I am going to use Magento 2 0.74.0-beta3 release. This release does not include the fix which simplifies the Payment class so it is going to be a good refactoring exercise.

The Payment class implements Magento\Sales\Api\Data\OrderPaymentInterface interface as part of a Module Service Contract (a.k.a. Module API) of the Magento\Sales module. The interface provides getters and setters for manipulations with payment related information. In addition to this, the Payment class provides service methods which help to process order payment information. Here is the list of service methods:

Pic. 3. The Magento\Sales\Model\Order\Payment public methods.

This diagram shows only public methods which are defined in the Payment class. All getter and setter methods are out of the diagram.

The Payment class has a total of 2595 lines of code. This class-that-can-do-any-operation-with-payment class has code smell. This Large Class is difficult to read, understand, and troubleshoot.

We are going to review the Payment::registerPaymentReviewAction() method as one of the most complicated and complex in implementation.

[php]
 /**
     * Perform the payment review action: either initiated by merchant or by a notification
     *
     * Sets order to processing state and optionally approves invoice or cancels the order
     *
     * @param string $action
     * @param bool $isOnline
     * @return $this
     * @throws \Exception
     * @SuppressWarnings(PHPMD.CyclomaticComplexity)
     * @SuppressWarnings(PHPMD.NPathComplexity)
     */
    public function registerPaymentReviewAction($action, $isOnline)
    {
        $order = $this->getOrder();
        $transactionId = $isOnline ? $this->getLastTransId() : $this->getTransactionId();
        $invoice = $this->_getInvoiceForTransactionId($transactionId);
        // invoke the payment method to determine what to do with the transaction
        $result = null;
        $message = null;
        switch ($action) {
            case self::REVIEW_ACTION_ACCEPT:
                if ($isOnline) {
                    if ($this->getMethodInstance()->setStore($order->getStoreId())->acceptPayment($this)) {
                        $result = true;
                        $message = __('Approved the payment online.');
                    } else {
                        $result = -1;
                        $message = __('There is no need to approve this payment.');
                    }
                } else {
                    $result = (bool)$this->getNotificationResult() ? true : -1;
                    $message = __('Registered notification about approved payment.');
                }
                break;
            case self::REVIEW_ACTION_DENY:
                if ($isOnline) {
                    if ($this->getMethodInstance()->setStore($order->getStoreId())->denyPayment($this)) {
                        $result = false;
                        $message = __('Denied the payment online');
                    } else {
                        $result = -1;
                        $message = __('There is no need to deny this payment.');
                    }
                } else {
                    $result = (bool)$this->getNotificationResult() ? false : -1;
                    $message = __('Registered notification about denied payment.');
                }
                break;
            case self::REVIEW_ACTION_UPDATE:
                if ($isOnline) {
                    $this->getMethodInstance()->setStore(
                        $order->getStoreId()
                    )->fetchTransactionInfo(
                        $this,
                        $transactionId
                    );
                }
                if ($this->getIsTransactionApproved()) {
                    $result = true;
                    $message = __('Registered update about approved payment.');
                } elseif ($this->getIsTransactionDenied()) {
                    $result = false;
                    $message = __('Registered update about denied payment.');
                } else {
                    $result = -1;
                    $message = __('There is no update for the payment.');
                }
                break;
            default:
                throw new \Exception('Not implemented.');
        }
        $message = $this->_prependMessage($message);
        if ($transactionId) {
            $message = $this->_appendTransactionToMessage($transactionId, $message);
        }
        // process payment in case of positive or negative result, or add a comment
        if (-1 === $result) { // switch won't work with such $result!
            if ($order->getState() != \Magento\Sales\Model\Order::STATE_PAYMENT_REVIEW) {
                $status = $this->getIsFraudDetected() ? \Magento\Sales\Model\Order::STATUS_FRAUD : false;
                $order->setState(\Magento\Sales\Model\Order::STATE_PAYMENT_REVIEW, $status, $message);
                if ($transactionId) {
                    $this->setLastTransId($transactionId);
                }
            } else {
                $order->addStatusHistoryComment($message);
            }
        } elseif (true === $result) {
            if ($invoice) {
                $invoice->pay();
                $this->_updateTotals(['base_amount_paid_online' => $invoice->getBaseGrandTotal()]);
                $order->addRelatedObject($invoice);
            }
            $order->setState(\Magento\Sales\Model\Order::STATE_PROCESSING, true, $message);
        } elseif (false === $result) {
            if ($invoice) {
                $invoice->cancel();
                $order->addRelatedObject($invoice);
            }
            $order->registerCancellation($message, false);
        }
        return $this;
    }
[/php]

The second name of this method is “Oh my gosh!”. Even PHPDocs says absolutely nothing. But let’s see what this method actually does:

1. Prepares and accepts online payment that is in review state
2. Denies online payment
3. Updates online payment
4. Processes payment result
5. Prepares and sets order status and message
6. Initiates invoice payment
7. Adds status history comment to an order
8. Adds the ‘processing’ state to an order
9. Adds the ‘payment_review’ state to an order
10. Initiates an order cancellation
And, if we miss or misspell some method’s responsibility it is only because the method is hard to read.

When I searched for method usages I’ve found that the second method argument $isOnline is always set to true. It means that the method processes online payments only. Here I see 2 ways of refactoring:

1. Eliminate the method logic related to offline payment processing
2. Use “Extract Method” refactoring to move all offline payment processing logic into a separate method.

To be continued

In the next “Test Driven Development in practice – Part 2” article I am going to share Unit testing technique as well as some helpful tips. We will also finish step-by-step refactoring of the Magento\Sales\Model\Order\Payment class method with refactoring patterns like “Move Method”, “Extract Method”, “Remove Parameter” and others.

Stay in touch…

Next article: Test Driven Development in Practice – Part 2

I am looking forward for your feedback and any comments on the next topics you want to read from me.