maka82 commented on revision r45 in project apache-rat-pd.
Details are at http://code.google.com/p/apache-rat-pd/source/detail?r=45
Line-by-line comments:
File: /trunk/src/main/java/org/apache/rat/pd/core/PlagiarismDetector.java
(r45)
===============================================================================
Line 102: pdCommandLine, out);
-------------------------------------------------------------------------------
Comment by egor.pasko, Aug 05 (5 days ago):
a tiny formatting note:
this probably looks better if wrapped after '= 'like this:
final List<IHeuristicChecker> algorithmsForChecking =
configureHeuristicCheckers(pdCommandLine, out);
I totally agree. :)
a big formatting note:
indentation with tabs is scary, with verbosity of java as a language a
tab- or 8space- >indentation does not allow to place much on a line. We
may discuss this in ore details >elsewhere, there is no urgent need to
reformat everything now, of course :)
I use default Eclipse built-in formatter. If it is better to use any
different formatter, lets do it!
Line 130: * @param pdCommandLine
-------------------------------------------------------------------------------
Comment by egor.pasko, Aug 05 (5 days ago):
I guess another @param should be documented here too.
It is nice that you notice that. It is already done.
Actually, there is need to add/update javadoc in whole project
Line 135: List<ISearchEngine> toret = new
ArrayList<ISearchEngine>();
-------------------------------------------------------------------------------
Comment by egor.pasko, Aug 05 (5 days ago):
hm, 'ret' is more natural to me than 'toret', 'toRet' is comprehensible
too .. java people just love >CamelCase
This is correct. I like CamelCase, too! :)
Line 154: PdCommandLine pdCommandLine, PrintStream out) throws
IOException {
-------------------------------------------------------------------------------
Comment by egor.pasko, Aug 05 (5 days ago):
hey, code writers should be lazy. Repeating the 'out' constructor
parameter is a lot of work to >write and, more importantly, to read.
Probably, right now it is too late to feel lazy, but, yet, I would like
to advocate the approach of >setting the output stream later after
construction. Do we need to be logging something in the >constructors?
Do you agree?
Actually, if we want to use logging this way, PrintStream can be passed by
[1]setter method
[2]through class constructor
[3]like public static class member
I prefer first two approach . I think that situation in this class will be
much better if all methods are not static anymore. PrintStream will be then
just a class member. main function will be begin with:
PlagiarismDetector pd = new PlagiarismDetector();
.................
final PrintStream out = pd.getProperPrintStream(pdCommandLine);
and so on...
Line 167: .getLimit(), out));
-------------------------------------------------------------------------------
Comment by egor.pasko, Aug 05 (5 days ago):
Do we need to be logging something in the constructors?
No, we don't...
Line 336: new ByteArrayOutputStream());
-------------------------------------------------------------------------------
Comment by egor.pasko, Aug 05 (5 days ago):
huh, this dummy output stream will be taking space, right?
how about:
new PrintStream(new OutputStream() {
public void write(int b) {} // Do nothing when printing is requested.
});
This is nice hack. :) Thanks.
Respond to these comments at
http://code.google.com/p/apache-rat-pd/source/detail?r=45
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings
