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?
How to refactore big Data classes?
Moderator: General Moderators
-
agilezencd
- Forum Newbie
- Posts: 1
- Joined: Sat Mar 21, 2009 5:23 am
- Christopher
- Site Administrator
- Posts: 13596
- Joined: Wed Aug 25, 2004 7:54 pm
- Location: New York, NY, US
Re: How to refactore big Data classes?
In my opinion 8 constructor parameters are better than 8 setters if all 8 data are required. If any of them are optional then reducing the number of constructor parameters might make sense.
But the bigger question is: What are all of those parameters? It is possible that this class really has that many dependencies. But it makes me think that many of those parameters might be put behind a couple of Facades. More probable is that the whole system is poorly designed -- probably more Procedural than OO where the objects are really steps and not responsibilities.
Also, iIf you have a number of commonly used objects (e.g. $numbers, $contacts, $groups, $campaigns, $user) then perhaps a Registry would clean up access to these.
But the bigger question is: What are all of those parameters? It is possible that this class really has that many dependencies. But it makes me think that many of those parameters might be put behind a couple of Facades. More probable is that the whole system is poorly designed -- probably more Procedural than OO where the objects are really steps and not responsibilities.
Also, iIf you have a number of commonly used objects (e.g. $numbers, $contacts, $groups, $campaigns, $user) then perhaps a Registry would clean up access to these.
(#10850)
Re: How to refactore big Data classes?
Procedural is dead, long live proceduralarborint wrote:But the bigger question is: What are all of those parameters? It is possible that this class really has that many dependencies. But it makes me think that many of those parameters might be put behind a couple of Facades. More probable is that the whole system is poorly designed -- probably more Procedural than OO where the objects are really steps and not responsibilities.
Read that sentence again, would you? You should ask yourself: what operation(s) are you talking about and is it really a "parser class"'s responsibility to calculate the cost for this operation? Try to think about what you're trying to accomplish by thinking about the responsibilities and behaviour, instead of parameters, variables and data.agilezencd wrote:To count the total cost for some operation, we have to pass many kind of information to the "parser class".
EDIT: It seems fair to tell everyone that I too have struggled with these situations, simply because the transition of my mindset wasn't yet accomplished. OO is a different way of thinking, not just syntax.