13 Nov
2006
13 Nov
'06
5:15 p.m.
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
13 Nov
13 Nov
5:50 p.m.
New subject: [xwiki-dev] [Code review] ZipExplorer plugin
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
6:15 p.m.
New subject: [xwiki-dev] [Code review] ZipExplorer plugin
-----Original Message-----
From: jeremi23(a)gmail.com [mailto:jeremi23@gmail.com] On Behalf Of
jeremi joslin
Sent: lundi 13 novembre 2006 17:51
To: xwiki-dev(a)objectweb.org
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. * 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/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. *
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.
6:30 p.m.
New subject: [xwiki-dev] [Code review] ZipExplorer plugin
On 11/13/06, Vincent Massol <vincent(a)massol.net> wrote:
I don't understand this. Apparently the algorithm is:
- find the string after the action
- count 4 "/" and return the remaining portion
yes beacause we are using an URL like :
http://mavenbook.xwiki.com/xwiki/bin/download/Main/Source/mavenbook-1.0.zip…
we want to get Directory/File.txt
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.
Yes, it's just I didn't know that was in this lib. Yes, it should be
done maybe not on the plugin, but on XWikiAttachement.
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.
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. No it's not. We don't have defined rules for this. so some code
we are
doing like this, some code no. We know we should work on this to
define a generic way to handle this, it's more a time issue.
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).
Yes, but for the moment we can't. So it's more an issue of time. If we
had more time, we can improve this.
-----Original Message-----
From: jeremi23(a)gmail.com [mailto:jeremi23@gmail.com] On Behalf Of
jeremi joslin
Sent: lundi 13 novembre 2006 17:51
To: xwiki-dev(a)objectweb.org
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.
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?
Yes, we should have add a javadoc about this.
[snip]
If you could explain to me what the plugin does then I'll add it.
I'll do
it later, I don't have time now.
* 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/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. *
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.
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.
let's
go.
jeremi
9:33 p.m.
New subject: [xwiki-dev] [Code review] ZipExplorer plugin
-----Original Message-----
From: jeremi23(a)gmail.com [mailto:jeremi23@gmail.com] On Behalf Of
jeremi joslin
Sent: lundi 13 novembre 2006 18:30
To: xwiki-dev(a)objectweb.org
Subject: Re: [xwiki-dev] [Code review] ZipExplorer plugin
[snip]
I don't
understand this. Apparently the algorithm is:
- find the string after the action
- count 4 "/" and return the remaining portion
yes beacause we are using an URL like :
http://mavenbook.xwiki.com/xwiki/bin/download/Main/Source/mavenbook-
1.0.zip/Directory/File.txt
we want to get Directory/File.txt 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?
Yes, we should have add a javadoc about this. > > *
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 > > *
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.
Hmm... So you're saying that in all xwiki code we're not handling > > *
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 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.
let's go.
14 Nov
14 Nov
9:32 a.m.
New subject: XWikiURL (was RE: [xwiki-dev] [Code review] ZipExplorer plugin=
-----Original Message-----
From: jeremi23(a)gmail.com [mailto:jeremi23@gmail.com] On Behalf Of
jeremi joslin
Sent: lundi 13 novembre 2006 17:51
To: xwiki-dev(a)objectweb.org
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 have thought more about this and I still believe the best would be to create a XWikiURL
class. The reason is that I was looking at randome at the code and noticed the following
in downloadAction:
public String getFileName(String path, String action) {
path = path.substring(path.indexOf("/" + action));
int pos = 0;
for (int i = 0; i < 3; i++) {
pos = path.indexOf("/", pos + 1);
}
if (path.indexOf("/", pos + 1) > 0)
return path.substring(pos + 1, path.indexOf("/", pos + 1));
return path.substring(pos + 1);
}
Thus the code in the zipexplorer plugin is actually very close to being a copy paste...
:-)
What it suggest to me is that there's a domain in xwiki which is URL management and we
need a class for it. I'd suggest a XWikiURL class and a XWikiZipURL one for managing
Zip URLs which are an extension of standard xwiki URLs. Note that this would remove the
copy paste as XWikiZipURL.getFileName() could share the code with XWikiURL.getFileName().
WDYT?
[snip]
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
15 Nov
15 Nov
11:43 a.m.
New subject: XWikiURL (was RE: [xwiki-dev] [Code review] ZipExplorer plugin=
Hi,
As we talk yesterday, it can be done in XWikiURLFactory.
We should add functions to parse an URL and return as you said an XWikiURI.
It will fix some problems of JSR168 integration.
jeremi
On 11/14/06, Vincent Massol <vincent(a)massol.net> wrote:
--
jeremi
-----Original Message-----
From: jeremi23(a)gmail.com [mailto:jeremi23@gmail.com] On Behalf Of
jeremi joslin
Sent: lundi 13 novembre 2006 17:51
To: xwiki-dev(a)objectweb.org
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 have thought more about this and I still believe the best would be to create a XWikiURL
class. The reason is that I was looking at randome at the code and noticed the following
in downloadAction:
public String getFileName(String path, String action) {
path = path.substring(path.indexOf("/" + action));
int pos = 0;
for (int i = 0; i < 3; i++) {
pos = path.indexOf("/", pos + 1);
}
if (path.indexOf("/", pos + 1) > 0)
return path.substring(pos + 1, path.indexOf("/", pos + 1));
return path.substring(pos + 1);
}
Thus the code in the zipexplorer plugin is actually very close to being a copy paste...
:-)
What it suggest to me is that there's a domain in xwiki which is URL management and
we need a class for it. I'd suggest a XWikiURL class and a XWikiZipURL one for
managing Zip URLs which are an extension of standard xwiki URLs. Note that this would
remove the copy paste as XWikiZipURL.getFileName() could share the code with
XWikiURL.getFileName().
WDYT?
[snip]
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
--
You receive this message as a subscriber of the xwiki-dev(a)objectweb.org mailing list.
To unsubscribe: mailto:xwiki-dev-unsubscribe@objectweb.org
For general help: mailto:sympa@objectweb.org?subject=help
ObjectWeb mailing lists service home page: http://www.objectweb.org/wws
6809
days inactive
6811
days old
6 comments
2 participants
participants (2)
-
jeremi joslin -
Vincent Massol