[xwiki-dev] [Code review] ZipExplorer plugin

[jeremi joslin]

On 11/13/06, Vincent Massol <vincent at massol.net> wrote:
> Hi,
>
> In order to understand better the code I have reviewed the ZipExplorer
> plugin. Here's my code review:
>
> * There are 2 unused fields that should be removed: capacity and log
for sure.

>
> * The getFileName() method can be improved:
>   - first parameter variable must not be modified (the path one in this
> case). There should always be a local variable created.
>   - I don't think XWiki URI manipulation should be done in this class at
> all! I'd recommend creating a XWikiURI class in charge of performing all
> actions on XWiki URI: creation, getting the different part, etc. In this
> manner is XWiki decides to change the URI format, it'll only be done in one
> place. This would also remove completely the need for getFileName() from
> ZipExplorerPlugin.
yes, it's not good, but the zip explorer plugin is doing something
special, not standart on xwiki, so we just have this to do it. I know
this plugin is not working for exemple in exo.

>
> * 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.

>
> * 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/io/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.

>
> * 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.
>
> * 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.

>
> * 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.


jeremi