-----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of jeremi joslin Sent: lundi 13 novembre 2006 17:51 To: [email protected] Subject: Re: [xwiki-dev] [Code review] ZipExplorer plugin
[snip]
* 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.
I don't understand this. Apparently the algorithm is: - find the string after the action - count 4 "/" and return the remaining portion 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? [snip]
* 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.
* 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.
* 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.
* 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.
* 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). 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. 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