Active
Project:
Facet API
Version:
7.x-2.x-dev
Component:
Documentation
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
25 Oct 2011 at 16:10 UTC
Updated:
29 Dec 2011 at 16:46 UTC
I've taken some time to analyze and draw the relationships in this module so people would or could have an easier understanding of what is happening under the hood whenever they try to code for this module
Some comments :
- There are a number of loop-references between the adapter and it's relations. I think this could be avoided by thinking the architecture through (see the references that have 2 lines to each other)
- The variable facet in FacetapiFacet might be a bit too un-descriptive and it looks like it could be renamed to "settings" or "facet_settings"
Also I've generated doxygen documentation with loads of diagrams and easy to read documentation. In addition to the attached images it should make the facetapi module easier to understand. The documentation can be found on http://facetapi.nickveenhof.be
| Comment | File | Size | Author |
|---|---|---|---|
| ClassDiagram_facetapi.png | 111 KB | nick_vh | |
| Classes in FacetApi.png | 216.04 KB | nick_vh |
Comments
Comment #0.0
nick_vhtesting image field
Comment #0.1
nick_vhUpdated issue summary.
Comment #0.2
nick_vhUpdated issue summary.
Comment #0.3
nick_vhUpdated issue summary.
Comment #1
cpliakas commentedThis is excellent! Thanks for putting this together.
Comment #2
cpliakas commentedOne question I have is about the "loop-references". One of the loops is between the FacetapiAdapter and FacetapiFacet classes. Multiple FacetapiFacet classes are instantiated by the adapter, and the adapter passes itself as a parameter since some of the methods in the adapter are needed. If we broke this pattern we would have to the FacetapiFacet methods into the adapter, store all of the FacetapiFacet data in the adapter keyed by the facet, and then add contextual parameters to the methods determining which facet we are acting on. One of the benefit of OO code is the ability to store data that is local only to the instance of that class. Breaking this paradigm would add a lot of logic and complexity to the adapter which is effectively eliminated by allowing multiple FacetapiFacet class instances. I am definitely for cleaning up the architecture where it makes sense, I just don't want to add complexity where it isn't needed. I am curious to see some patches in terms of how the loops could be broken in a way that improved the overall architecture.
Thanks for raising these points,
Chris
Comment #3
cpliakas commentedMarking as postponed. This would be a great exercise after starting the 2.x branch.
Comment #4
cpliakas commentedMoving to the 2.x branch.
Comment #4.0
cpliakas commentedchanged target