Too big data objects

Question

Hello.

Recently, I have identified very strong code smell in one of the project, I'm working for.

Especially, in its cost counting feature. To count the total cost for some operation, we have to pass many kind of information to the "parser class".

For example:

• phone numbers
• selected campaigns
• selected templates
• selected contacts
• selected groups
• and about 2-4 information types more

Before refactoring there were all this parameters were passed through constructor to the Counter class(8 parameters, you can image that..).

To increase readability I have introduced a Data class, named CostCountingData with readable getters and setters to all this properties.

But I don't think, that that code became much readable after this refactoring:

$cost_data = new CostCountingData();
$cost_data->setNumbers($numbers);
$cost_data->setContacts($contacts);
$cost_data->setGroups($groups);
$cost_data->setCampaigns($campaigns);
$cost_data->setUser($user);
$cost_data->setText($text);
$cost_data->setTotalQuantity($total_quantity);

$CostCounter = new TemplateReccurentSendingCostCounter($cost_data);  
$total_cost = $CostCounter->count();

Can you tell me whether there is some problem with readability of this code snippet? Maybe you can see any code smells and can point me at them..

The only idea I have how to refactore this code, is to split this big data object to several, consisting related data types. But I'm not sure whether should I do this or not..

Whay do you think about it?

Answer 1

If what you want is named parameters (which it looks to me is what you are trying to achieve), would you ever consider just passing in an associative array, with the names as keys? There is also the Named Parameter Idiom, I can only find a good reference for this for C++ though, perhaps it's called something else by PHP programmers.

For that, you'd change your methods on CostCountingData so they all returned the original object. That way you could rewrite your top piece of code to this:

$cost_data = new CostCountingData();
$cost_data
 ->setNumbers($numbers)
 ->setContacts($contacts)
 ->setGroups($groups)
 ->setCampaigns($campaigns)
 ->setUser($user)
 ->setText($text)
 ->setTotalQuantity($total_quantity);

So there's two options. I would probably go for the named parameter idiom myself as it is self-documenting, but I don't think the difference is too great.

Answer 2

If I needed the data object (and classes called "data" are themselves a smell) I'd set its values in its constructor. But the interesting question is where are these values coming from? The source of the values should itself be an object of some sort, and its probably the one you are really interested in.

Edit: If the data is coming from POST, then I would make the class be wrapper around the POST data. I would not provide any setter functions, and would make the read accessors look like this (my PHP is more than a little rusty):

class CostStuff {

   constructor CostStuff( $postdata ) {
       $mypost = $postdata;
   }

   function User() {
      return $mypost[ "user_name" ];
   }

   ...

}

Answer 3

What do you think is wrong with your code snippet? If you have several variables that you've already used that you now need to get into $cost_data, then you'll need to set each one in turn. There's no way around this.

A question though, is why do you have these variables $numbers, $contracts, etc. that you're now deciding you need to move into $cost_data ?

Is the refactoring you're looking for possibly that the point in the code where you set $numbers should actually be the point you're setting $cost_data->numbers, and having the $numbers variable at all is superfluous?

E.g.

$numbers=getNumbersFromSomewhere() ; 
// do stuff
$contracts=getContracstFromSomewhere() ;
// do stuff 
$cost_data=new dataObject() ; 
$cost_data->setNumbers($numbers);
$cost_data->setContracts($contracts) ; 
$cost_data->someOperation() ;

becomes

$cost_data=new dataObject() ; 

$cost_data->setNumbers(getNumbersFromSomewhere()) ; 
// do stuff
$cost_data->setContracts(getContractsFromSomewhere()) ; 
// do stuff
$cost_data->someOperation() ;

Answer 4

There are a few possibilities here:

• If you only need a few of the parameters each time, the named parameter approach suggested elsewhere in this answer is a nice solution.

• If you always need all of the parameters, I would suggest that the smell is coming from a broken semantic model of the data required to calculate a cost.

No matter how you dress up a call to a method that takes eight arguments, the fact that eight supposedly unrelated pieces of information are required to calculate something will always smell.

Take a step back:

• Are the eight separate arguments really all unrelated, or can you compose intermediate objects that reflect the reality of the situation a little better? Products? invoice items? Something else?

• Can the cost method be split into smaller methods that calculate part of the cost based on fewer arguments, and the total cost produced from adding up the component costs?