0
\$\begingroup\$

I have a utility class, called Bills, that extracts some information from a billId and a paymentId. I'm pasting the whole class here for completeness, you don't need to understand the algorithm behind all functions to be able to answer my question.

<?php

class Bills {
    private $typeAndServicePaymentIdMap = [
        0 => 13,
        1 => 4, // water
        2 => 1, // electricity
        3 => 5, // gas
        4 => 2, // landline
        5 => 3, // cellphone
        6 => 6, // municipality due
        7 => 6, // municipality due type 7
        9 => [
            'default' => 24, // traffic due
            '001' => 38, // traffic fine
            '002' => 38, // traffic fine
        ],
    ];

    private $logger;

    public function __construct(File_Logger $logger)
    {
        $this->logger = $logger;
    }

    public function getType($billId)
    {
        return substr($billId, -2, 1);
    }

    public function getAmount($paymentId)
    {
        $paymentId = str_pad($paymentId, 13, '0', STR_PAD_LEFT);
        $amount = ltrim(substr($paymentId, 0, 8), '0') . '000';

        return $amount;
    }

    public function getCompanyCode($billId)
    {
        return substr($billId, -5, 3);
    }

    public function getPaymentServiceId($billId)
    {
        $type = $this->getType($billId);

        if (key_exists($type, $this->typeAndPaymentServiceIdMap)) {
            return $this->calculatePaymentServiceId($type);
        } else {
            $this->logger->errorLog('Type ' . $type . ' is not defined in the $typeAndPaymentServiceIdMap.');
        }
    }

    private function calculatePaymentServiceId($type)
    {
        $paymentServiceId = $this->typeAndPaymentServiceIdMap[$type];

        if (is_array($paymentServiceId)) {
            return $this->createPaymentServiceIdFromCompanyCode($paymentServiceId);
        } else {
            return $paymentServiceId;
        }
    }

    private function createPaymentServiceIdFromCompanyCode($paymentServiceId)
    {
        $companyCode = $this->getCompanyCode($billId); // TODO: fix this, we don't have $billId here

        if (key_exists($companyCode, $paymentServiceId)) {
            return $paymentServiceId[$companyCode];
        } else {
            return $paymentServiceId['default'];
        }
    }
}

The problem I have is in getPaymentServiceId(). Here is what this function does:

  1. If the type is found in the type -> paymentServiceId map, it checks the value defined for $type in the map. For example, if $type is 1, it gets the value for 1, which in this case is 4.
  2. If the paymentServiceId (the value that was retrieved in the previous step) is not an array, it just returns it. For example, if type is 1, the value is 4, and it is returned as-is.
  3. However, if the paymentServiceId is an array, we need to calculate the company code (calculated using $this->getCompanyCode()) to be able to get a paymentServiceId, and as you can see, to do that, we need billId. In the current mapping, only a type of 9 would result in this calculation. All other types would not require this.

In other words, if createPaymentServiceIdFromCompanyCode() is called, it needs billId. If not, billId is not needed. My problem is that I don't want to pass billId as an argument down the chain when it is only needed for one $type.

Is there a better way of structuring this class to achieve what I want, or do I have to just pass billId down the chain every time?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

If you want to avoid passing $billId argument down the chain within Bills class - "stub" chain of those methods.
Let the getPaymentServiceId() method do all the "processing" work. Remove calculatePaymentServiceId and createPaymentServiceIdFromCompanyCode methods.
The final result should look like the following:

class Bills {

    private $typeAndPaymentServiceIdMap = [
        0 => 13,
        1 => 4, // water
        2 => 1, // electricity
        3 => 5, // gas
        4 => 2, // landline
        5 => 3, // cellphone
        6 => 6, // municipality due
        7 => 6, // municipality due type 7
        9 => [
            'default' => 24, // traffic due
            '001' => 38, // traffic fine
            '002' => 38, // traffic fine
        ],
    ];
    private $logger;

    public function __construct($logger = "") {
        $this->logger = $logger;
    }

    public function getType($billId = "") {
        return substr($billId, -2, 1);
    }

    public function getAmount($paymentId = "") {
        $paymentId = str_pad($paymentId, 13, '0', STR_PAD_LEFT);
        $amount = ltrim(substr($paymentId, 0, 8), '0') . '000';

        return $amount;
    }

    public function getCompanyCode($billId = "") {
        return substr($billId, -5, 3);
    }

    public function getPaymentServiceId($billId = "") {
        if (!$billId) {
            $this->logger->errorLog('billId is required!');
            return;
        }
        $type = $this->getType($billId);
        $companyCode = $this->getCompanyCode($billId);

        if (key_exists($type, $this->typeAndPaymentServiceIdMap)) {
            $paymentServiceId = $this->typeAndPaymentServiceIdMap[$type];
            if (is_array($paymentServiceId)) {
                return (key_exists($companyCode, $paymentServiceId))? $paymentServiceId[$companyCode] : $paymentServiceId['default'];
            } else {
                return $paymentServiceId;
            }
        } else {
            $this->logger->errorLog('Type ' . $type . ' is not defined in the $typeAndPaymentServiceIdMap.');
        }
    }
}

// $logger object
$bill = new Bills($logger);

var_dump($bill->getPaymentServiceId("00190"));  // int 38
var_dump($bill->getPaymentServiceId("00390"));  // int 24
\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot! The reason I broke that method down into two more methods is the if statements. I don't want to have two if statement inside each other and am looking to bring down the complexity. \$\endgroup\$ Commented Mar 16, 2016 at 5:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.