New subject: [xwiki-dev] Some desing issues with XWiki

30 Apr 2007

Hi all,

I know everyone is really busy at this time trying to polish XWiki for
the 1.0 release, so this may not be a good time to start discussing 
design issues. Thus feel free to ignore this message until the bug 
chase is over ;)

Here I want to discuss about some IMO design flaws in XWiki and suggest
some ways to improve them. I know the programmer's motto that goes
"If it is not broken, don't fix it". While I usually agree with this,
sometimes I believe some changes can improve code readeability,
isolation and encourage better coding practices. Well now without
further ado, some things I believe we should fix:

1)  com.xpn.xwiki.XWiki has become a mess. It has become the official 
"put the method that does not fit elsewhere" place. Thus it is 
gigantic and complex. 

2) there are many abstraction violations in XWiki. For example, storage.
XWiki is modular, that's great, everything about storage  
goes in com.xpn.xwiki.store and there are nice interfaces that abstract
the underlying storage for the rest of the world. 
Sadly this is not true, for example com.xpn.xwiki.XWiki imports 
XWikiHibernateStore, XWikiJcrStore, etc. So we loose the isolation.
Far worse, take a look at the methods:
   getHibernateStore()
   isSQL()
   updateDatabase()
   ...

I mean why are those methods in XWiki.java instead of in some store
class. Those methods are encouraging bad code. Because 
those methods are making assumptions about the XWiki storage.
And the problem with them is that they tend to promote
code like 3)

3) Reinventing OO:
   I better explain this with an example, this is a method in XWiki.java:

      public List getSpaceDocsName(String spaceName, XWikiContext context) 
         throws XWikiException
      {
         List docs = null;
         if (getNotCacheStore() instanceof XWikiHibernateStore) {
            docs = this.search("select distinct
               doc.name from XWikiDocument doc",
               new Object[][] {{
               "doc.web",
               spaceName}},
               context);
         } else if (getNotCacheStore() instanceof XWikiJcrStore)
         {
            docs = ((XWikiJcrStore)
                getNotCacheStore()).getSpaceDocsName(spaceName,
                context);
         }

   What are we doing here, we are doing a special case depending on
   the store subclass... Sadly this is done in many places in XWiki,
   so when I decide to add a new storage, I have to go to all of these
   places and add another else if statement. No way!
   Why not just do a dispatching call, that is what OO is for ;)

Ok, so I really think we should start fixing this for XWiki 2.0.
What I propose for the above points:
     1) Instead of adding new methods to XWiki.java, try to fit
        them in a more appropriate place or even create a new class.

     2) Start working on real isolation. To support old versions, 
        mark the "bad methods" as deprecated and provide nice interfaces
        that do not make assumptions about the implementation.
        I'm happy, for storage I think Artem Melentyev is going to 
        improve his QueryPlugin during Soc, so this will be great ;)

     3) Factor the hand-made dispatchs scattered in the XWiki codebase,
        with dispatching.

I believe this is a first step before moving towards component based 
architecture.

I have tried to tackle some of these problems, here is a patch. 
Do not hesitate to criticize it, it is not supposed to be anything 
else than a proof of concept because I think design issues should
really be discussed before doing any drastic changes.
I have tested it and believe it does not break anything, yet I
have a small doubt, maybe some XWiki expert can enlighten me:

in xwiki.setUserDefaultGroup() a call to saveXWikiObject hibernate
specific method is done.

The question is is the following equivalent and store agnostic ?
 getDocument + addObject + saveDocument <=> getDocument + addObject +
saveXWikiObject

If not how are we supposed to save the Object when we are not using hibernate ?

Thanks,

Pablo