HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Payment function with too many parameters

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
paymentwithfunctiontoomanyparameters

Problem

I have a method with too many parameters.

How would you refactor it?

Things I considered so far:

  • pass an array to the object and check for required keys (no code hint, phpdoc, programmer doesn't know which are required)



  • pass an object (silly method with getters and setters, programmer still doesn't know which are required)



Here's the method:

```
public function doDirectPayment(
$amount,
$credit_card_type,
$credit_card_number,
$expiration_month,
$expiration_year,
$cvv2,
$first_name,
$last_name,
$address1,
$address2,
$city,
$state,
$zip,
$country,
$currency_code,
$ip_address,
$payment_action = 'Sale'
)
{
$client = $this->getClient();

$client->setParameterGet('METHOD', 'DoDirectPayment');

$month = str_pad(ltrim((string)$expiration_month, 0), 2, '0', STR_PAD_LEFT);
$expiration_date = $month . $expiration_year;

$client->setParameterGet('PAYMENTACTION', urlencode($payment_action));
$client->setParameterGet('AMT', urlencode($amount));
$client->setParameterGet('CREDITCARDTYPE', urlencode($credit_card_type));
$client->setParameterGet('ACCT', urlencode($credit_card_number));
$client->setParameterGet('EXPDATE', urlencode($expiration_date));
$client->setParameterGet('CVV2', urlencode($cvv2));
$client->setParameterGet('FIRSTNAME', urlencode($first_name));
$client->setParameterGet('LASTNAME', urlencode($last_name));
$client->setParameterGet('STREET', urlencode($address1));

if (!empty($address2)) {
$client->setParameterGet('STREET2', urlencode($address2));
}

$client->setParameterGet('CITY', urlencode($city));
$client->setParameterGet('STATE', urlencode($state));
$client->setParameterGet('ZIP', urlencode($zip));
$client->setParameterGet('COUNTRYCODE', urlencode($country));
$client->setParameterGet('CURRENCYCODE', urlencode($currency_code));
$client->setParameterGet('IPADDRESS', urlencode($ip_address));

Solution

I'd group the parameters:

doDirectPayment(CreditCard $card, User $user, array $options = array())


But this doesn't solve all problems.

To mark something required you'd use interface, but the interface can only force the existence of the methods, not the properties.

So the best option seems to be forcing the params in the constructor:

class CreditCard {
     public function __construct($required1, $required2 /*, etc*/);
}


But this way you can't instantiate the object without providing options (no fluent interface).

You may mark the params optional and provide some validation method. The problem is, how to automatically call this method. Maybe some SPL interface may help here.

From Fowler's Refactoring:


Long Parameter Lists


Too many parameters. Functionality is wrong, or not enough use of
fields


Hinders: comprehension, use, adding parameters


Main Refactoring: Replace Parameter with Method, Preserve Whole Object, Introduce Parameter Object

Looks like most of those params will come from the user filled form, so you could simply do:

doDirectPayment(PaymentForm $form, array $otherOptions = array()) {
    // ...
    foreach ($form->getValues() as $name => $value) {
        $client->setParameterGet(strtoupper($name), urlencode($value));
    }
    // ...
}

Code Snippets

doDirectPayment(CreditCard $card, User $user, array $options = array())
class CreditCard {
     public function __construct($required1, $required2 /*, etc*/);
}
doDirectPayment(PaymentForm $form, array $otherOptions = array()) {
    // ...
    foreach ($form->getValues() as $name => $value) {
        $client->setParameterGet(strtoupper($name), urlencode($value));
    }
    // ...
}

Context

StackExchange Code Review Q#3733, answer score: 8

Revisions (0)

No revisions yet.