0
\$\begingroup\$

I'm integrating my app with a payment gateway, and have the following code which returns an array to populate the transaction info:

function populate_transaction_info($data)
{
  extract($this->extract_model_data($data));

  // set to empty array to ensure run-able code
  $transaction_info = [];

  return array_merge($transaction_info, [
    'pan' => $card_data['card_number'],
    'expdate' => $this->build_expiry_date_string($card_data['card_exp']),
    'cust_id' => $person['email'],
    // ... more params here, not relevant for question
  ]);
}

// shown for context / run-ability
function extract_model_data($data)
{
  return [
    'person' => $data['Person'],
    'card_data' => $data['CreditCard']],
  ];
}

My question pertains to the build_expiry_date_string(), defined as:

function build_expiry_date_string($card_expiry)
{
  return $card_expiry['month'] . substr($card_expiry['year'], -2);
}

I'm passing $card_data['card_exp'] into the function, but I'm wondering if the function itself should be the only thing that knows that it needs to look in the card_exp index of the array? So for example, should I refactor as:

...
'expdate' => $this->build_expiry_date_string($card_data),
...

function build_expiry_date_string($card_data)
{
  return $card_data['card_exp']['month'] . substr($card_data['card_exp']['year'], -2);
}

If yes, why?

\$\endgroup\$
3
\$\begingroup\$

Your first version of build_expiry_date_string() will be able to build an shorter notation date from any date array. I would therefore rename it to build_short_date_string().

The second version actually is named correctly, but can only convert an expiry date to the shorter notation.

Which is the better version?

Well, clearly, if you need to create a short date string from more than one date array then the first one wins. The second one is more specialized. It will only work for the card_exp date.

In general I don't see the point of overspecializing functions. The more general purpose version does a good job, so I would prefer that one, even if it is, for now, only used for one date array.

Functions should be used to encapsulate functionality that can be reused and therefore make your code easier to maintain. (reference)

It is the reusability that wins for me here, over any other considerations.

Note that we're talking about simple functions here, not methods that are part of classes. OOP is a whole other can of worms.

Some other remarks about your code (this is code review afterall):

  • You extract() data at the beginning of populate_transaction_info(). This seems like a nice trick, but it makes your code harder to read and debug. For instance, I have to assume that $person is one of these extracted variables, but for me it appears out of thin air. You could help your code reader by using a prefix whenever your extract something: extract(...., EXTR_PREFIX_ALL, 'model'); and then use $model_person, but preferably just explicitly define the person variable in the function.
  • Your comment: "set to empty array to ensure run-able code" and the subsequent array_merge() make no sense to me. Why not just define the array?
\$\endgroup\$
  • 1
    \$\begingroup\$ Amen to not endorsing extract(). I have never had a reason to use, nor do I ever hope to use extract() in any project, ever. \$\endgroup\$ – mickmackusa May 28 at 11:07
  • \$\begingroup\$ Thanks for the feedback, I really appreciate it! I will rename and keep the first version for re-usability. I also knew I was going to get called out for using extract, heh. I will be more explicit about pulling the appropriate variables out of $data. Finally, the comment was just for this example... $transaction_info is initialized with some data called from another function, but I didn't feel it was super necessary to include here. Sorry for the confusion. \$\endgroup\$ – Erebus May 28 at 13:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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