13 Nov
2006
13 Nov
'06
5:50 p.m.
On 11/13/06, Vincent Massol <vincent(a)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