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, <[email protected]> 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
