You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@creadur.apache.org by co...@google.com on 2009/08/18 14:03:34 UTC
Comment on revision r45 in apache-rat-pd
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
Re: Comment on revision r45 in apache-rat-pd
Posted by Robert Burrell Donkin <ro...@blueyonder.co.uk>.
Egor Pasko wrote:
> On Wed, Aug 19, 2009 at 10:35 PM, Robert Burrell
> Donkin<ro...@blueyonder.co.uk> wrote:
>> Egor Pasko wrote:
>>> Hello, rat-dev folks,
>>>
>>> if you think code reviews pollute this list, please, speak up, we will
>>> remove it from CC. We tried this CC to make sure code review comments
>>> by Marija will in fact reach me (how evil:). Now we know that on the
>>> revisions that I starred, the comments will go directly to me.
>> the problem is that all of these comments and commit messages need to
>> moderated through by hand (AIUI google uses address munging incompatible
>> with ezalm meaning that each has to be passed specifically). this makes
>> it a complete PITA for the moderators.
>>
>> this really needs forwarding setup (rather than direct subscription) so
>> that all the mail originates from a single address. if you have any
>> problems sorting this out, let me know and i'll give you a hand.
>
> Robert, all,
>
> moderation issue with ezmlm and codesite-noreply@google.com was a
> surprise to me. I am really sorry for bothering you with this. For
> reviews we are switching to using a separate list on google groups.
> Anyone will be able to subscribe.
i came as a surprise so me too
the commits also needed to be moderated through for similar reasons
> If I set up a gmail filter to forward review emails to rat-dev this
> will probably work fine (i.e. not requiring moderation), right?
> Anyway, it is not critical because SoC is near ending, unless we have
> more than 50 other interesting reviews, I won't consider forwarding.
yes
- robert
Re: Comment on revision r45 in apache-rat-pd
Posted by Egor Pasko <eg...@gmail.com>.
On Wed, Aug 19, 2009 at 10:35 PM, Robert Burrell
Donkin<ro...@blueyonder.co.uk> wrote:
> Egor Pasko wrote:
>> Hello, rat-dev folks,
>>
>> if you think code reviews pollute this list, please, speak up, we will
>> remove it from CC. We tried this CC to make sure code review comments
>> by Marija will in fact reach me (how evil:). Now we know that on the
>> revisions that I starred, the comments will go directly to me.
>
> the problem is that all of these comments and commit messages need to
> moderated through by hand (AIUI google uses address munging incompatible
> with ezalm meaning that each has to be passed specifically). this makes
> it a complete PITA for the moderators.
>
> this really needs forwarding setup (rather than direct subscription) so
> that all the mail originates from a single address. if you have any
> problems sorting this out, let me know and i'll give you a hand.
Robert, all,
moderation issue with ezmlm and codesite-noreply@google.com was a
surprise to me. I am really sorry for bothering you with this. For
reviews we are switching to using a separate list on google groups.
Anyone will be able to subscribe.
If I set up a gmail filter to forward review emails to rat-dev this
will probably work fine (i.e. not requiring moderation), right?
Anyway, it is not critical because SoC is near ending, unless we have
more than 50 other interesting reviews, I won't consider forwarding.
--
Egor Pasko
Re: Comment on revision r45 in apache-rat-pd
Posted by Robert Burrell Donkin <ro...@blueyonder.co.uk>.
Egor Pasko wrote:
> Hello, rat-dev folks,
>
> if you think code reviews pollute this list, please, speak up, we will
> remove it from CC. We tried this CC to make sure code review comments
> by Marija will in fact reach me (how evil:). Now we know that on the
> revisions that I starred, the comments will go directly to me.
the problem is that all of these comments and commit messages need to
moderated through by hand (AIUI google uses address munging incompatible
with ezalm meaning that each has to be passed specifically). this makes
it a complete PITA for the moderators.
this really needs forwarding setup (rather than direct subscription) so
that all the mail originates from a single address. if you have any
problems sorting this out, let me know and i'll give you a hand.
- robert
Re: Comment on revision r45 in apache-rat-pd
Posted by Egor Pasko <eg...@gmail.com>.
Hello, rat-dev folks,
if you think code reviews pollute this list, please, speak up, we will
remove it from CC. We tried this CC to make sure code review comments
by Marija will in fact reach me (how evil:). Now we know that on the
revisions that I starred, the comments will go directly to me.
So, thanks, and I hope this was an interesting read.
P.S.: to participate in reviews you will need committer status in apache-rat-pd.
On Tue, Aug 18, 2009 at 4:03 PM, <co...@google.com> wrote:
> 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
>
--
Egor Pasko