patternphpMinor
Payment function with too many parameters
Viewed 0 times
paymentwithfunctiontoomanyparameters
Problem
I have a method with too many parameters.
How would you refactor it?
Things I considered so far:
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));
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:
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:
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(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.