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 Wed, 19 Aug 2009 06:51:04 GMT
egor.pasko commented on revision r45 in project apache-rat-pd.
Details are at http://code.google.com/p/apache-rat-pd/source/detail?r=45

Score: Positive


Line-by-line comments:

File: /trunk/src/main/java/org/apache/rat/pd/core/PlagiarismDetector.java  
(r45)
===============================================================================

Line 102: 				pdCommandLine, out);
-------------------------------------------------------------------------------
> >this probably looks better if wrapped after '= 'like this:
> >final List<IHeuristicChecker> algorithmsForChecking =
> >    configureHeuristicCheckers(pdCommandLine, out);

> I totally agree. :)

thanks for fixing this in some next commit

> I use default Eclipse built-in formatter. If it is better to use any  
> different formatter, lets do it!

This was surprising to me. I never really used default formatting in  
Eclipse, so I did not know. Here is how to fix:
Window -> Preferences -> Java -> Code Style -> Formatter -> Select a  
Profile (Eclipse [built-in]) Show ... -> Indentation -> Tab policy ->  
Spaces only -> Apply

name it something like "Eclipse [spaces]"

Then use Source -> Format to reformat individual files. Worked for me :)

Do you like that?

To reformat everything a quick replacement script can be run. A usual  
request in such cases is not to intermix formatting changes with other work  
in a single commit.

Line 130: 	 * @param pdCommandLine
-------------------------------------------------------------------------------
> It is nice that you notice that. It is already done.

thanks for doing this! it makes the code cleaner

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! :)

hey :) you could give a link on the revision where you fix this. I checked  
the last revision. Sounds to be fixed. Thanks for that!

Line 154: 			PdCommandLine pdCommandLine, PrintStream out) throws  
IOException {
-------------------------------------------------------------------------------
> 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);

I am OK with this approach.

However, to make code shorter you could make a public setter with a  
protected getter in a superclass of all heuristics. And then you do not  
need to define this thing in every heuristic. And cleaning up is easier.

Just a thought.

Line 336: 				new ByteArrayOutputStream());
-------------------------------------------------------------------------------
> This is nice hack. :) Thanks.

I am happy to help you, and this is not a hack :)

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