Some desing issues with XWiki
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
Hi Pablo, Thanks for this great email. I completely agree. This is exactly the reason for XWiki 2.0 architecture actually :) If you check the 2.0 architecture page, you'll see I have started listing the components required. I think there shouldn't be any need for a XWiki object at all after we do this. Another thing to consider: we might need a presentation layer 1) to make it easy for the scripting code 2) to make the scripting code small and simple and 3) so that most presentation code is in Java. Right now one reason XWiki, XWikiDocuments, etc are big with lots of stuff inside is because they don't differentiate presentation layer needs from API needs. I haven't had the time to check your patch. I'd like to start planning for 2.0 sometime next week or the week after. We have several options, one of which (which is probably the best is a "big bang strategy with backports when possible"). Thanks again for your precious help. I understand we're not working on the same time frame so we need to get started on the 2.0 architecture in SVN so that people like us can start helping out Chers, -Vincent On Apr 30, 2007, at 11:51 AM, Pablo Oliveira wrote:
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 <patch>
-- You receive this message as a subscriber of the xwiki- [email protected] mailing list. To unsubscribe: mailto:[email protected] For general help: mailto:[email protected]?subject=help ObjectWeb mailing lists service home page: http://www.objectweb.org/ wws
On Apr 30, Vincent Massol wrote :
Another thing to consider: we might need a presentation layer 1) to make it easy for the scripting code 2) to make the scripting code small and simple and 3) so that most presentation code is in Java. Right now one reason XWiki, XWikiDocuments, etc are big with lots of stuff inside is because they don't differentiate presentation layer needs from API needs.
Yes, that would be nice indeed.
I'd like to start planning for 2.0 sometime next week or the week after. We have several options, one of which (which is probably the best is a "big bang strategy with backports when possible").
What do you exactly mean by "big bang strategy" ?
I haven't had the time to check your patch.
That's good ;) I did not send my latest diff by mistake, so here is the new one. Sorry for the noise. Thanks, Pablo
On Apr 30, 2007, at 12:51 PM, Pablo Oliveira wrote:
On Apr 30, Vincent Massol wrote :
Another thing to consider: we might need a presentation layer 1) to make it easy for the scripting code 2) to make the scripting code small and simple and 3) so that most presentation code is in Java. Right now one reason XWiki, XWikiDocuments, etc are big with lots of stuff inside is because they don't differentiate presentation layer needs from API needs.
Yes, that would be nice indeed.
I'd like to start planning for 2.0 sometime next week or the week after. We have several options, one of which (which is probably the best is a "big bang strategy with backports when possible").
What do you exactly mean by "big bang strategy" ?
Start from scratch instead of trying to modify something existing. -Vincent
I haven't had the time to check your patch.
That's good ;) I did not send my latest diff by mistake, so here is the new one. Sorry for the noise.
Thanks,
Pablo <patch>
-- You receive this message as a subscriber of the xwiki- [email protected] mailing list. To unsubscribe: mailto:[email protected] For general help: mailto:[email protected]?subject=help ObjectWeb mailing lists service home page: http://www.objectweb.org/ wws
participants (2)
-
Pablo Oliveira -
Vincent Massol