7
\$\begingroup\$

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));

    $response = $client->request(Zend_Http_Client::GET);

    return $response;
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ biggest argument list I've ever seen :) \$\endgroup\$
    – webarto
    Jul 29 '11 at 21:52
8
\$\begingroup\$

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));
    }
    // ...
}
\$\endgroup\$
1
  • \$\begingroup\$ +1 the constructor of the data object that will be the parameter causes required fields, then privatizing the setters on those ensures relative immutability so they can't be replaced with nulls. This is exactly the correct approach from my experience. \$\endgroup\$ Jul 30 '11 at 23:48
5
\$\begingroup\$

Passing an object (class name 'Payment' perhaps?) is not that silly actually. You can add public method validate() to it, that could be called from doDirectPayment() in order to check if all required fields have been filled.

You will probably want to make it as value object so as to avoid unexpected side effects.

\$\endgroup\$
1
  • \$\begingroup\$ also, if you use an object, return $this from the setters so function calls can be chained. But the array solution is equally fine, I guess it's a matter of taste.. \$\endgroup\$
    – yi_H
    Jul 29 '11 at 20:30
0
\$\begingroup\$

I'm not entirely sure if this is feasible in PHP, but a class-seperated builder pattern is a nice way to alleviate long parameter lists.

You can have each builder return the next component as required.

ie:

CreditCardInfo.Build(credit_card_type, _number, month, year) // Returns NameInfo Builder class
.Build(first_name, last_name) //Returns AddressInfo Builder class
.Build(address1, address2, city, state, zip, country) //Returns TransactionInfo Builder class
.Build(ip_address) //Returns Payment class
.DoDirectPayment();

Each of the builder classes { CreditCardInfo, NameInfo, AddressInfo, TransactionInfo } have a Build method that returns the next Builder class in order. The final one returned, Payment, will contain the DoDirectDeposit action.

This helps show the intent of each of the parameters, and gives a clear API as to how each parameter grouping contributes to the requirements of the function call.

\$\endgroup\$

Your Answer

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