CleanCode is not what you think

I have worked with a lot of different people in my life, and some people don’t rely on Code Quality as a success factor, and people think their code is clean, but it isn’t.

I will put a simple example, a class that imports orders from WooCommerce. At first glimpse it seems to be enough clean:

final class WooIntegration extends WooBase3
{
    public function importOrders(): void
    {
        $wooOrders = $this->retrieveWooOrders();
        foreach ($wooOrders as $wooOrder) {
            try {
                $this->importOneOrder($wooOrder);
            } catch (Exception $e) {
                $this->process->fail($e->getMessage());
            }
        }
    }
......

We retrieve the orders from WooCommerce, and then iterate the array to import orders one by one.

In case of error, we will mark the process as failed.

The import Order seems to be clean too:

private function importOneOrder(array $wooOrder): void
{
    $order = new Order();
    $order->provider_id = $this->connection->provider_id;
    $order->shipping_address = $wooOrder['shipping']['address'];
    ......
    $order->save();
    $this->importLines($order, $wooOrder);
}

And importing lines is also very easy:

private function importLines(Order $order, array $wooOrder): void
{
    foreach ($wooOrder['items'] as $wooLine) {
        $sku = $wooLine['sku'];
        $product = Inventory::find()->andWhere(['sku' => $sku, 'integration_id' => $this->connection->integrationId]);
        $orderLine = new OrderLine();
        $orderLine->order_id = $order->id;
        $orderLine->qty = $wooLine['qty'];
        $orderLine->gtin = $product->gtin;
        $orderLine->save();
    }
}

What’s wrong with this code?

The first wrong thing is that is hard to do unit testing. How can I test import one order? There is no easy way because importWooOrder is private.

The main problem is that we failed in the first letter of SOLID, Single Responsibility. This class does too much:

• Retrieve Orders from woocommerce
• Iterate
• Add Order
• Add Line

So, if we move ImportOneOrder to another class, we will have something like this:

public function importOrders(): void
{
    $service = new ImportOneWooOrderService();
    $wooOrders = $this->retrieveWooOrders();
    foreach ($wooOrders as $wooOrder) {
        try {
            $service->importOneOrder($wooOrder);
        } catch (Exception $e) {
            $this->process->fail($e->getMessage());
        }
    }
}

Now we can test the importOneOrder, but we have one line that we have to solve:

$order->provider_id = $this->connection->provider_id;

In the new class, we don’t have the connection property.
We may think to add the “connection” property to new class. Doing this, we will violate another SOLID principle, complicating testing.

When developing a functionality, we need to reduce the knowledge and dependence on external objects. So, if we only need the provider_id, let’s add the parameter to the method:

private function importOneOrder(array $wooOrder, int $providerId): void

At this point we can test importOneOrder, we only need the Woo Order and the providerId. Easy? No, we have another problem.

Let’s review again import OrderLine:

private function importLines(Order $order, array $wooOrder): void
{
    foreach ($wooOrder['items'] as $wooLine) {
        $sku = $wooLine['sku'];
        $product = Inventory::find()->andWhere(['sku' => $sku, 'integration_id' => $this->connection->integrationId]);
        $orderLine = new OrderLine();
        $orderLine->order_id = $order->id;
        $orderLine->qty = $wooLine['qty'];
        $orderLine->gtin = $product->gtin;
        $orderLine->save();
    }
}

In this code, we retrieve the product from the database by Sku. So, unit testing won’t work. We need to “inject” a service. The I of SOLID.

private function importLines(Order $order, array $wooOrder, InventoryServiceInterface $inventoryService): void
{
    foreach ($wooOrder['items'] as $wooLine) {
        $sku = $wooLine['sku'];
        $product = $inventoryService->retriveBySkuAndIntegrationId($sku, $this->connection->integrationId);

If we do this, the method is expecting a service as aparameter, that will return the product. To test, we can use something like Mockery, or custom InventoryServiceInterface implementation.

$inventoryService = Mockery::mock(InventoryServiceInterface::class);
$inventoryService->shouldReceive('retriveBySkuAndIntegrationId')->andReturn(InventoryMother::randomWithSku($sku));
$importOneOrderService->importOneOrder($order,$providerId,$inventoryService);

We have created a Mock of the interface and intercepted the method call for retrieving an Inventory Item.

But this is not the end of the refactory needed:

1. Now, the ImportOneWooOrderService is updating directly the database, we need another Dependency Injection of a repository, that will have a store($order) method.
2. We are not checking if $inventoryService->retriveBySkuAndIntegrationId is returning a product. Is very common that devs don’t check that.
3. $order->save() in most frameworks can fail, and devs aren’t used to checking if it went well. Yes, most devs don’t check if it got saved. I’m talking about Laravel, Yii, etc…

Summary

• Clean Code is not only developing legible code.
• Code must be easy to test.
• Code must take into account all conditions; not found, not saved, etc…
• Code must be decoupled and with small knowledge on external classes.
• If you don’t know SOLID, is your turn.