maven-doxia-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vincent Siveton" <vincent.sive...@gmail.com>
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 Thu, 27 Jul 2006 22:58:28 GMT
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 ;)
IMHO I think that Doxia needs to be review at large to add comment.

>
> [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 ;)

>
> >
> >      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.

[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...?

[snip]

> > +    protected String getString( String key )
> > +    {
> > +        if ( StringUtils.isEmpty( key ) )
> > +        {
> > +            throw new IllegalArgumentException( "The key cannot be empty" );
> > +        }
> > +
> > +        return i18n.getString( "book-renderer", Locale.getDefault(), key ).trim();
> > +    }
>
> This should probably be moved to the i18n component or at least to a
> I18nUtil.

+1

Thanks!

Vincent

Mime
View raw message