Github user nickwallen commented on the issue:

    https://github.com/apache/metron/pull/970
  
    I dug into this further.  I think fundamentally there are a few issues 
causing problems.
    
    1. The extensive inheritance amongst all of these interfaces is 
problematic.  This introduces hard dependencies and is difficult to make sense 
of.  This picture does not look clean to me; IMHO.
    
        <img 
src="https://user-images.githubusercontent.com/2475409/38499370-537d4f12-3bd5-11e8-8e1b-8171630e8adf.jpeg";
 width=200>
    
        For example, why is an `UpdateDao` also a `RetrieveLatestDao`?  These 
need to be completely separate.  There should be no inheritance between the 
two.  There should be a `SolrUpdateDao -> UpdateDao` and a 
`SolrRetrieveLatestDao -> RetrieveLatestDao`.  
    
        If someone needs to do updates, they should use the `UpdateDao`.  If 
someone needs to get the latest documents, they should the `RetrieveLatestDao`. 
 
    
        The same logic can  be applied to the other interfaces.
    
    1. I believe all of this inheritance, along with all of the default 
methods, exist because we are using inheritance for code reuse, instead of 
composition.  This prevents it from being more modular, simple, and testable.  
    
        For example, the `SolrMetaAlertDao` should **use** a 
`RetrieveLatestDao`, it should **not be** a `RetrieveLatestDao`.
    
       Another example, if the `SolrUpdateDao` needs to also search, then it 
should be passed an instance of a `SearchDao` to do that.  It does not itself 
need to be a `SearchDao`. 
    
    1. There are also unnecessary interfaces that cause confusion.
    
        For example, there are compositional uses of the interface `IndexDAO`.  
But what is an `IndexDAO`?  Based on the inheritance hierarchy an `IndexDAO` is 
an `UpdateDao`, a `SearchDao`, a `RetrieveLatestDao` all at the same time.  
    
       If something needs to do updates, then pass in an `UpdateDao`.  If 
something needs to search, then pass in a `SearchDao`.  If something needs to 
do BOTH, then pass in both a `SearchDao` and an `UpdateDao`.  
    
       The only real method in `IndexDao` seems exactly the same as a 
`ColumnMetadataDao`.  So if something needs that functionality, it should be 
given a `ColumnMetadataDao` not an `IndexDao`.
    
       The `IndexDao` should either be given some real purpose or die a quick 
and painless death
    
    
    
    



---

Reply via email to