On 11/13/06, Vincent Massol <[email protected]> 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