Pages

Wednesday, October 29, 2014

Indaface!



Recently my team leader gave me an S.L.A.P, hehehe...
Don't worry this is not a case of bullying, it just a way of refering to an important Object Oriented Principle known as the Single Level of Abstraction Principle.
As Usual at the office paring on an story, when we got to the refactoring bit, I was told to improve a method that had some ugly conditional logic on it, and also some duplication.
Instead of removing the duplication, I just delegated all the problematic part to another method, so the original method would look smaller and cleaner, but...
He said to me: Do you think that code you just delegated is located now at a different level of abstraction?... Indaface! Violation of S.L.A.P
Below, an example that somehow recreates todays funny situation :P Sorry, can't show you the real code(Dont wanna get in trouble :) ).

 /*  
     We have a vault that its being populated and categorized but there is a little 
     duplication issue when populating the vault with CAT-3 items. Items withGrook() 
     are definitely CAT-3, but those withTrook() are only categorized as CAT-3 if 
     klop.isHigh().   
 */  
    public Vault stuff(Klop klop, Vault vault) {  
     vault.include("CAT-1", azra());  
     vault.include("CAT-2", khy());  
     vault.include("CAT-3", things.stream().filter(withGrook()).
     map(toSponge()).map(toPlik(klop)).collect(toList()));  
     if(klop.isHigh())  
       vault.include("CAT-3", things.stream().filter(withTrook()).
       map(toSponge()).map(toPlik(klop)).collect(toList()));  
     return vault;  
     }  
 /*  
     At the beggining we may feel tempted to extract that vault.include, to its own
     method which will be specific to CAT-3 items, regardles that the stuff method looks
     shorter, the problem is that we did not remove the duplication plus we are disrespecting
     the S.L.A.P principle.  
 */      
      public Vault stuff(Klop klop, Vault vault) {  
           vault.include("CAT-1", azra());  
           vault.include("CAT-2", khy());  
           includeCat3Items(vault,klop);  
           return vault;  
        }  

   private void includeCat3Items(Vault vault, Klop klop) {  
           if(klop.isHigh())  
             vault.include("CAT-3", things.stream().filter(withGrook()).
             map(toSponge()).map(toPlik(klop)).collect(toList()));  
             vault.include("CAT-3", things.stream().filter(withTrook()).
             map(toSponge()).map(toPlik(klop)).collect(toList()));  
   }  
  
  /*If we want to respect S.L.A.P, in this case what we need to extract, is just the 
    changing part, and delegate the condition to check if klop.isHigh, to the delegate
     method.  
  */  
     public Vault stuff(Klop klop, Vault vault) {  
     vault.include("CAT-1", azra());  
     vault.include("CAT-2", khy());  
     vault.include("CAT-3", things.stream().filter(ook(klop)).
     map(toSponge()).map(toPlik(klop)).collect(toList()));  
     return vault;  
   }  

   private Predicate<String> ook(Klop klop) {  
     return klop.isHigh() ? withGrook(): withTrook();  
   }  

 /*  
     The final refactor could even go one step further by extracting implementation 
     detail into a plu(Klop klop) method  
 */      
   public Vault stuff(Klop klop, Vault vault) {  
     vault.include("CAT-1", azra());  
     vault.include("CAT-2", khy());  
     vault.include("CAT-3", plu(klop));  
     return vault;  
   }
  
   private List<Object> plu(Klop klop) {  
     return things.stream().filter(ook(klop)).map(toSponge()).map(toPlik(klop)).collect(toList());  
   }  
   private Predicate<String> ook(Klop klop) {  
     return klop.isHigh() ? withGrook(): withTrook();  
   }  


This post is dedicated to L.K, thanks for the patience :)

Share with your friends