[xwiki-dev] [Code review] ZipExplorer plugin

[Vincent Massol]

> -----Original Message-----
> From: jeremi23 at gmail.com [mailto:jeremi23 at gmail.com] On Behalf Of
> jeremi joslin
> Sent: lundi 13 novembre 2006 18:30
> To: xwiki-dev at objectweb.org
> Subject: Re: [xwiki-dev] [Code review] ZipExplorer plugin

[snip]

> > I don't understand this. Apparently the algorithm is:
> > - find the string after the action
> > - count 4 "/" and return the remaining portion
> 
> yes beacause we are using an URL like :
> http://mavenbook.xwiki.com/xwiki/bin/download/Main/Source/mavenbook-
> 1.0.zip/Directory/File.txt
> we want to get Directory/File.txt

One more question: do you think the getFileName() algorithm could be modified so that it looks for a ".zip" string instead of "/"+action? That would simplify the algorithm and remove the dependency on the xwiki action.

> > But if I take a standard XWiki URL such as
> http://mavenbook.xwiki.com/xwiki/bin/download/Main/Source/mavenbook-
> 1.0.zip then what is it supposed to return? The action is "download"
> and there aren't 4 "/" after it. BTW if there were some unit tests for
> this method it would have been easier to understand.
> >
> > Also if it's not working in exo I think it should be documented in
> the javadoc of the plugin and/or a jira issue opened, don't you think?
> 
> Yes, we should have add a javadoc about this.

Done + unit tests added for this getFileName() method.

BTW what do you think about naming the method getFileNameFromZipURL()? It looks more explicit to me.

> > > > * There are no javadocs... The minimum would be to have a class
> > > > javadoc explaining what the plugin is doing and putting an
> example of
> > > how to use it.
> > > it could be good.
> >
> > If you could explain to me what the plugin does then I'll add it.
> I'll do it later, I don't have time now.

Ok

> > > > * readFromInputStream() is a generic method to read an input
> stream
> > > in
> > > > memory. Note that this method should be moved in some generic
> place
> > > as
> > > > it would be useful for other portion of the code and is
> completely
> > > generic.
> > > > Even better this method is already implemented in commons-io
> > > > [http://jakarta.apache.org/commons/io/api-
> > > release/org/apache/commons/i
> > > > o/File Utils.html#readFileToByteArray(java.io.File)]. Also note
> that
> > > > it's a dangerous method as it's limited by the size of the memory
> and
> > > > the size of of a byte[] array in Java... If the zip is large it's
> > > > going to fail... It would be better to implement it with
> streaming
> > > > instead, if possible.
> > > The first problem is that attachments us an array of bytes and not
> a
> > > stream. If I remember well what ludovic tells me about it is that
> > > hibernate does not support it.
> >
> > Ok. At least we could use commons-io, no? And then we should
> definitely document in the javadoc why we're not using a stream and the
> ensuing limitation. I can do this.
> Yes, it's just I didn't know that was in this lib. Yes, it should be
> done maybe not on the plugin, but on XWikiAttachement.

I'll do it a bit later once I understand more about the plugin.

> > > > * getFileTreeList(). By reading this method's name I don't
> understand
> > > > what it does. I also don't understand what is the difference with
> > > > getFileList() (I haven't analyzed the code).
> > > the first one return a list which is for generating a tree, and the
> > > second one just the flat file list.
> >
> > So both return a list. I still don't understand what's different in
> both method calls... If you have an example then I could possibly write
> unit tests with it.

Any example?

> >
> > > > * Exception handling is very very poor :-) For example:
> > > >
> > > >             } catch (XWikiException e) {
> > > >                 e.printStackTrace();
> > > >             } catch (IOException e) {
> > > >                 e.printStackTrace();
> > > >             }
> > > In velocity there is no way to catch the exception, so if we send
> an
> > > exception to the page, it brake all the page, so I prefer to not
> raise
> > > the exception. Maybe the best sould be to set a variable with the
> error
> > > and be able to read the list of error.
> >
> > Hmm... So you're saying that in all xwiki code we're not handling
> exceptions because of this? I think we should definitely have some
> error page with the description of the error.
> No it's not. We don't have defined rules for this. so some code we are
> doing like this, some code no. We know we should work on this to
> define a generic way to handle this, it's more a time issue.

Ok. I've created an issue for this: 
http://jira.xwiki.org/jira/browse/XWIKI-412

> > > > * The unit tests seem to be lacking. In any case they're hard to
> > > read.
> > > > I haven't fully understood them yet... I believe they shouldn't
> test
> > > > XWiki's core but only the plugin.
> > > Not everything is tested because it's too long to write a test for
> some
> > > of this functions(cactus test). We have junit test, but not cactus
> > > test.
> >
> > I don't think we should do cactus tests at all actually. Unit tests
> are the best for plugins as when we test a plugin the goal is not to
> test the xwiki core. We should also have functional tests for general
> tests but the majority of tests should be unit tests (i.e. in isolation
> from the environment - no container, not database and when possible no
> files).
> 
> Yes, but for the moment we can't. So it's more an issue of time. If we
> had more time, we can improve this.

The problem is that it gets harder and harder if you don't look into this and funnily in the end it takes more time not to handle it as you then have to handle more manual tests, more bug fixes, you have to explain it to others, others are loosing time when trying to understand the code, etc. Actually for example I've spent about 2-3 hours on the zipexplorer plugin today. If there had been javadoc and tests, I would have spent about 10 minutes to look at it. Multiply this time by the number of people who'll ever look at it and the figure gets huge quickly. Of course this doesn't even taken into account the fact that because of no javadoc and no tests people will help less, etc. :-)

Anyway I know how hard it is when you have delivery deadlines...

Let's just try to climb back the slope now ;-) I'll be very happy to help as much as I can.

> > I'm happy to help define how to right unit tests for xwiki. We could
> use ZipExplorerPlugin as a trial for writing them. It's probably going
> to require some modifications to the core to some degree. That's the
> good point of unit tests: they do improve design. Only problem is that
> xwiki technical debt is quite high at the moment, so it'll cost a lot
> for the first tests. It'll get better afterwards.
> let's go.

Cool!

-Vincent








___________________________________________________________________________
Découvrez une nouvelle façon d'obtenir des réponses à toutes vos questions !
Profitez des connaissances, des opinions et des expériences des internautes sur Yahoo! Questions/Réponses
http://fr.answers.yahoo.com