Coding: Pushing the logic back
I was reading a post on the law of demeter by Richard Hart recently and it reminded me that a lot of the refactorings that we typically do on code bases are about pushing the logic back into objects instead of exposing data and performing calculations elsewhere.
An example that I spotted where we did this recently was while building a 'BusinessSummary' object whose state was based on the state of a collection of other objects.
The code was keeping a count of the various states of the business objects:
public BusinessSummary buildSummaryObject() { BusinessSummary businessSummary = new BusinessSummary(); for(BusinessObject businessObject : businessObjects) { State state = businessObject.getState(); if(State.STATE_1.equals(state)) { businessSummary.incrementState1(); } else if(State.STATE_2.equals(state)) { businessSummary.incrementState2(); } else { // and so on } } return businessSummary; }
In this example 'State' is an enum representing the state the 'BusinessObject' is currently in.
Our first change to the code was to push the logic around state back into the 'BusinessObject' which results in the following code:
public BusinessSummary buildSummaryObject() { BusinessSummary businessSummary = new BusinessSummary(); for(BusinessObject businessObject : businessObjects) { if(businessObject.hasState1()) { businessSummary.incrementState1(); } else if(businessObject.hasState2()) { businessSummary.incrementState2(); } else { // and so on } } return businessSummary; }
public class BusinessObject { ... public boolean hasState1() { return State.STATE_1.equals(state); } ... }
I think this is an improvement but it still isn't following the tell don't ask principle, we're just asking to be told about the data instead of asking for the data itself.
We didn't have the time to do the next change where we would have got rid of the 'hasState' methods and just got the 'BusinessObject' to update the 'BusinessSummary' itself:
public BusinessSummary buildSummaryObject() { BusinessSummary businessSummary = new BusinessSummary(); for(BusinessObject businessObject : businessObjects) { businessObject.writeTo(businessSummary); } return businessSummary; }
public class BusinessObject { private final State state; ... public void writeTo(BusinessSummary businessSummary) { if(State.STATE_1.equals(state)) { businessSummary.incrementState1(); } else if(State.STATE_2.equals(state)) { businessSummary.incrementState2(); } else { // and so on } } ... }
Looking back on this code having written this post I wonder whether we need to do the loop or whether it would be better to pass the collection of 'businessObjects' to the 'BusinessSummary' and let it take care of that logic instead.
If we did that then we would need to expose 'hasState1′ and 'hasState2′ methods on 'BusinessObject' like on the first refactoring so that we could work out how to populate 'BusinessSummary'.
I prefer the solution where 'BusinessObject' writes to the 'BusinessSummary' although it seems like 'BusinessObject' now has more than one reason to change – if it changes or if 'BusinessSummary' changes. This would violate the single responsibility principle
I discussed this with Dave and he pointed out that as long as the contract that 'BusinessSummary' and 'BusinessObject' have – i.e. the 'incrementState1′ and 'incrementState2′ methods – stays the same then it wouldn't really be a problem.
There are other solutions. Having conditional logic is a smell. Having a state-object, which makes implicit classes of BusinessObjects may be a smell to use subclasses. If you want to count the states, you may use a hash with states as keys. The hash could be incremented inside the BusinessObjects or inside the summary. If the count is a domain-concept, you may use an own new class for the collection of BusinessObjects instead of the ArrayList.
ciao
jens
BTW: Your blog, your tactical insights are very inspiring!
Jens Himmelreich
11 Nov 09 at 10:47 pm
[...] This post was mentioned on Twitter by Mark Needham, planettw. planettw said: Mark Needham: Coding: Pushing the logic back: I was reading a post on the law of demeter by Richard Hart recently an… http://bit.ly/3xkULf [...]
Tweets that mention Coding: Pushing the logic back at Mark Needham -- Topsy.com
11 Nov 09 at 11:03 pm
Who is responsible for summarizing, and how do they get the data? There is no one right answer. If it's the business object, then it's doing too much. If it's the business summary, then it has to ask and not tell. It's always a tradeoff.
Michael L Perry
11 Nov 09 at 11:35 pm
Why not have BusinessSummary.increment(State) ?
Then, your interaction is more like the two classes passing requests and bits of information back-and-forth and BusinessSummary's API is less coupled to the various State instances. (excuse the short-hand pseudo-code below)
BusinessSummary.add(List objs) {
for(BusinessObject businessObject : objs){
businessObject.writeTo(businessSummary); }
}
BusinessObject.writeTo(BusinessSummary bs) {
bs.increment(this.state);
}
BusinessSummary.increment(State state) {
if (State.State1 == state) {etc…
}
I think the advantage here is that all the BusinessSummary really knows how to do is increment its State counter. All the rest is convenience methods.
Moandji Ezana
12 Nov 09 at 9:17 am
[...] Coding: Pushing the logic back – Mark Needham talks about refactoring business logic code back into business objects along with the tell don't ask, single responsibility principle and law of demeter development principles [...]
Reflective Perspective - Chris Alcock » The Morning Brew #475
12 Nov 09 at 6:27 pm
@Moandji – that might actually be even better, I like that. Neat.
Mark Needham
12 Nov 09 at 11:24 pm
@Mark: I know its asking instead of telling, but I find it much more natural to change from the initial code to:
public BusinessSummary buildSummaryObject() {
BusinessSummary businessSummary = new BusinessSummary();
for(BusinessObject businessObject : businessObjects) {
State state = businessObject.getState();
businessSummary.incrementState(state);
}
return businessSummary;
}
or:
public BusinessSummary buildSummaryObject() {
BusinessSummary businessSummary = new BusinessSummary();
for(BusinessObject businessObject : businessObjects) {
businessSummary.add(businessObject);
}
return businessSummary;
}
maybe even:
public BusinessSummary buildSummaryObject() {
BusinessSummary businessSummary = new BusinessSummary();
businessSummary.add(businessObjects);
}
Consider this is similar to @Moandji approach, but isn't a chatty comm i.e.
BusinessSummary.add(List objs) {
for(BusinessObject businessObject : objs){
businessObject.writeTo(businessSummary); // tell bo to write to self, so then bo tells self to increment … instead of just telling self to increment based on businessObject.getState()
}
}
For me its the business summary that should we caring about what information we are using for the summary, so I don't see why BusinessObject needs to be tied back to BusinessSummary.
freddy rios
13 Nov 09 at 1:30 am