The Author Online Book Forums are Moving

The Author Online Book Forums will soon redirect to Manning's liveBook and liveVideo. All book forum content will migrate to liveBook's discussion forum and all video forum content will migrate to liveVideo. Log in to liveBook or liveVideo with your Manning credentials to join the discussion!

Thank you for your engagement in the AoF over the years! We look forward to offering you a more enhanced forum experience.

phani.me1 (2) [Avatar] Offline
#1
I gained a lot of knowledge by reading Effective Unit Testing. I believe in the philosophy of clean design. However, sometimes strictly following design patterns suggested in the book do not answer 'practical-me'.

Please consider the following example which, I think, violates a bunch of design principles.

public class OfferEligibilityCheckerServiceImpl implements OfferEligibilityCheckerService, Refreshable{

private Map<String, OfferCriteria> offerIdToOfferCriteriaMap;

private OffersAccessorService offersAccessorService

public OfferEligibilityCheckerServiceImpl (OffersAccessorService offersAccessorService ){
this.offersAccessorService = offersAccessorService;
initValidOfferIdSet();
}

protected void initOfferIdToOfferCriteriaMap(){
offerIdToOfferCriteriaMap = offersAccessorService.get..Criteria();
}

//REAL BUSINESS LOGIC, i.e. this is why the service is used by clients!!
@Override
public boolean isUserEligible(String offerId, UserInfo userInfo){
offerCriteria = offerIdToOfferCriteriaMap.get(offerId);
return offerCriteria.isEligible(userInfo); // let's not worry about NPE
}


// Gets invoked at regular intervals by some scheduler, say Spring.
@Override // from Refreshable
public void refresh(){ // ANOTHER responsibility
initOfferIdToOfferCriteriaMap();
}
}


According to the book, above code is wrong at so many levels but I'm lacking enough in-depth knowledge to convince others / practical-me that it is mediocre / not testable.

As per my limited knowledge, the problem with the above design is that it looks testable as certain parts can be substituted but it kind of violates all the 'testable design' guidelines suggested in the book.

The following is the list of my dilemmas.

1) Clean code enthusiast in me: Complex logic in constructor.

Practical-me: Nope, I'm invoking a protected method from the constructor which could be overridden if you ever need a test double.


2) Me: Law of demeter violation - Ask for exact things, not intermediary.

Practical-me: See the power of "Code to Interface". I'm passing in a serviceImpl whereas the constructor expects a Service. So I can always substitute while testing so that the serviceImpl doesn't really talk to a DAO / database during unit tests.


3) Me: Violation of SRP - Handles business logic, handles let me get my own stuff during my construction, handles let me refresh myself.

Practical-me: That's fine! I don't want to break this class into 3 classes and go through the overhead of scheduling them / wiring them.


4) Me: Mixing Business logic and object construction logic.

Practical-me: I don't even get you.

Actually there was no such battle within my self when I wasn't aware of testable design (of course most of my code was mediocre).

However, I have started my quest for clean code but I'm not able to find the right motivation when to design for testability.

It'd great if anyone here could provide me more insights into clean code as that could enable me in the future to make a better design choice.

Please note that I've posted the same question on stackoverflow but not completely satisfied with the response.

lkoskela (66) [Avatar] Offline
#2
Re: Impacts of violating SRP, Law of Demeter, etc on Testability
Hi Phani,

To me it seems that the code you posted above is testable. It might not be the best division of responsibilities you might come up with, but it's testable as it is.


1) Complex logic in constructor

It doesn't seem to be so complex as to make testing more difficult. For instance, what the constructor logic does can be easily influenced by the collaborating object that is passed in through the constructor.


2 Law of Demeter violation

I don't think it's a testability issue in this particular case. I do think there's a bit of LoD violation here as you take in OffersAccessorService as a dependency and later operate on the Map it returns from getCriteria.


3) SRP violation

What this class seems to do primarily is to "cache the offer criteria". If that's what we'd call the single responsibility of this class then "get stuff from cache" and "refresh cache" don't sound like big violations of the SRP.

Now, if we'd take the "cache" concept and emphasize it, we might end up with a design a bit like this:

// same as before
OffersAccessorService service = new OffersAccessorService();

// new intermediary, which takes care of caching and refreshing the cache
CachedReadOnlyOffers cache = new CachedReadOnlyOffers(service)

// (almost) the same class you posted above, without the need to refresh the cache
new OfferEligibilityCheckerService(cache)

Methods like isUserEligible(...) would now be seemingly straightforward delegations to the CachedReadOnlyOffers object, unless there are more complicated "checks" to be made.

Does this give you any new options or ideas what to do?


4) Mixing Business logic and object construction logic

Actually, I don't get you either smilie Could you elaborate which parts here are mixing business logic with construction logic?
phani.me1 (2) [Avatar] Offline
#3
Re: Impacts of violating SRP, Law of Demeter, etc on Testability
Thank you very much for your time.

I'm quite new to TDD and would like to follow design patterns in general.

I think I'm just over-engineering things as I strictly try to follow testability, SOLID guidelines and going a bit astray.

I think ability to apply them the right way comes with more practice smilie

I just devised that example and presented my two school of thoughts and your explanation gave clarity over the right path to choose.