maven-doxia-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Trygve Laugstøl <tryg...@apache.org>
Subject Re: svn commit: r426203 - in /maven/doxia/trunk/doxia-sandbox/doxia-book: ./ src/main/java/org/apache/maven/doxia/book/services/indexer/ src/main/java/org/apache/maven/doxia/book/services/renderer/ src/main/java/org/apache/maven/doxia/book/services/r
Date Fri, 28 Jul 2006 18:12:40 GMT
Vincent Siveton wrote:
> Hi Trygve,
> 
> I like your points :) My comments inside.
> 
> [snip]
> 
>> >
>> > +    /**
>> > +     * Default constructor
>> > +     *
>> > +     * @param sectionEntry
>> > +     */
>>  >      public BookIndexingSink( IndexEntry sectionEntry )
>>  >      {
>>  >          stack.push( sectionEntry );
>>  >      }
>>
>> These comments are pretty useless, I'd rather not sprinkle the code with
>> obvious comments.
> 
> Agree but if no comment, nobody thinks to had comment, specially for
> new and important updates. my2cents ;)

Not sure what you mean, please explain again.

> IMHO I think that Doxia needs to be review at large to add comment.

Agreed, we need to document the important parts.

>>
>> [snip]
>>
>> > -//    public void definedTerm()
>> > -//    {
>> > -//        type = TYPE_DEFINED_TERM;
>> > -//    }
>> > -//
>> > -//    public void figureCaption()
>> > -//    {
>> > -//        type = TYPE_FIGURE;
>> > -//    }
>> > -//
>> > -//    public void tableCaption()
>> > -//    {
>> > -//        type = TYPE_TABLE;
>> > -//    }
>> > +    //    public void definedTerm()
>> > +    //    {
>> > +    //        type = TYPE_DEFINED_TERM;
>> > +    //    }
>> > +    //
>> > +    //    public void figureCaption()
>> > +    //    {
>> > +    //        type = TYPE_FIGURE;
>> > +    //    }
>> > +    //
>> > +    //    public void tableCaption()
>> > +    //    {
>> > +    //        type = TYPE_TABLE;
>> > +    //    }
>>
>> Any special reason for this other than you identing it by accident?
> 
> Well, it is not an accident: it is the Eclipse formatter with the
> Maven Code Style
> (http://maven.apache.org/guides/development/guide-m2-development.html)
> Promise, I will try idea ASAP ;)

That has never been a standard that I can recall. Comment blocks always 
start on column 1.

>>
>> >
>> >      public void text( String text )
>> >      {
>> >          IndexEntry entry;
>> >
>> > -        switch( type )
>> > +        switch ( type )
>> >          {
>> >              case TITLE:
>> >                  this.title = text;
>> > @@ -137,15 +168,7 @@
>> >                  // Sanitize the id. The most important step is to 
>> remove any blanks
>> >                  // 
>> -----------------------------------------------------------------------
>> >
>> > -                String id = text;
>> > -                id = id.toLowerCase();
>> > -                id = id.replace( '\'', '_' );
>> > -                id = id.replace( '\"', '_' );
>> > -                id = id.replace( ' ', '_' );
>> > -
>> > -                // 
>> -----------------------------------------------------------------------
>> > -                //
>> > -                // 
>> -----------------------------------------------------------------------
>> > +                String id = HtmlTools.encodeId( text );
>>
>> Ah, I knew it was somewhere.
> 
> It is a new method ;)
> 
> [snip]
> 
>> >          {
>> >              if ( !context.getOutputDirectory().mkdirs() )
>> >              {
>> > -                throw new BookDoxiaException(
>> > -                    "Could not make directory: " + 
>> context.getOutputDirectory().getAbsolutePath() + "." );
>> > +                throw new BookDoxiaException( "Could not make 
>> directory: "
>> > +                    + 
>> context.getOutputDirectory().getAbsolutePath() + "." );
>> >              }
>> >          }
>> >
>> > -        // 
>> -----------------------------------------------------------------------
>> > -        //
>> > -        // 
>> -----------------------------------------------------------------------
>> > -
>>
>> I like these, they are separators between logical parts of the method.
> 
> It is not a Maven standard style thus I removed it. Is it for Doxia?
> If yes, we need a developping guide.

They are very much a standard part of the Maven code, we (at least I) 
use it all the time.

> [snip]
> 
>> > +
>> > +    // 
>> -----------------------------------------------------------------------
>> >      // Private
>> >      // 
>> -----------------------------------------------------------------------
>> >
>> > +    /**
>> > +     * Render the book, ie the book index and all chapter index
>> > +     *
>> > +     * @param book
>> > +     * @param context
>> > +     * @throws BookDoxiaException if any
>> > +     */
>> >      private void renderBook( BookModel book, BookContext context )
>> >          throws BookDoxiaException
>> >      {
>> > -        // 
>> -----------------------------------------------------------------------
>> > -        // Render the book index.xml page
>> > -        // 
>> -----------------------------------------------------------------------
>> > -
>> >          File index = new File( context.getOutputDirectory(), 
>> "index.xml" );
>> >
>> >          try
>> > @@ -86,12 +137,6 @@
>> >          }
>> >
>> >          // 
>> -----------------------------------------------------------------------
>> > -        // Render the index.html files for each chapter
>> > -        // 
>> -----------------------------------------------------------------------
>> > -
>> > -        // TODO: Implement
>> > -
>> > -        // 
>> -----------------------------------------------------------------------
>> >          // Render all the chapters
>> >          // 
>> -----------------------------------------------------------------------
>>
>> Ditto here about the commends. They explain the flow in the code.
> 
> IMHO javadoc should provide it ;) developing guide...?

No, javadoc is not the same thing as inline comment. This is documenting 
the _code itself_ not what the class/method is supposed to do.

[snip]

--
Trygve


Mime
View raw message