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
* 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.
* 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.
* 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.
* 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).
* Exception handling is very very poor :-) For example:
} catch (XWikiException e) {
e.printStackTrace();
} catch (IOException e) {
e.printStackTrace();
}
* 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.
Let me know what you think and I'll create some jira issues to fix it.
Thanks
-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
Hi there,
As a reminder, I'll start moving tests we have in trunk/src/tests in
core/src/test in a few hour's time. I'll move the unit tests first (the ones
with no environment dependency). I'll keep the Ant build working at all
times.
As usual let me know if this is going to cause an issue for you.
Thanks
-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
