creadur-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From codesite-nore...@google.com
Subject Comment on revision r45 in apache-rat-pd
Date Tue, 18 Aug 2009 12:03:34 GMT
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

Mime
View raw message