Re: svn commit: r757852 - /xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.java

2009-03-25 Thread Adrian Cumiskey

Hi Vincent,

Yes you are right it should be hidden (although it is still accessible 
through pageSeq.getRoot().getLayoutMasterSet().getSimplePageMaster()).
I just added these methods for convenience, but I have now found a 
better way to do this.  I will revert my changes.


Adrian.

Vincent Hennebert wrote:

Hi Adrian,

  

Author: acumiskey
Date: Tue Mar 24 15:39:15 2009
New Revision: 757852

URL: http://svn.apache.org/viewvc?rev=757852&view=rev
Log:
Added some nice bean methods for pageSequenceMaster and simplePageMaster, this 
also makes startOfNode() easier to read too.

Modified:

xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.java




I’m not sure I like this change. I tend to think that a class should not
expose public methods unless they are of immediate necessity. That helps
to reduce complexity and confusion for newcomers. If they prove to be
necessary they can always be added later on.

Also, in this case I don’t think any external object needs to know
whether the page masters for this page sequence were declared using
a single simple-page-master or a page-sequence-master. That knowledge
should be kept encapsulated inside the PageSequence class.

Finally, in the startOfNode method the get*Master method will be called
twice: once by has*Master to see if the master exists, and once to
actually get it. This is slightly overkill.

I think those new methods should at least be made private. Although the
original code simply looks fine to me...

WDYT?
Thanks,
Vincent

  




Re: svn commit: r757852 - /xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.java

2009-03-25 Thread Vincent Hennebert
Hi Adrian,

> Author: acumiskey
> Date: Tue Mar 24 15:39:15 2009
> New Revision: 757852
> 
> URL: http://svn.apache.org/viewvc?rev=757852&view=rev
> Log:
> Added some nice bean methods for pageSequenceMaster and simplePageMaster, 
> this also makes startOfNode() easier to read too.
> 
> Modified:
> 
> xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.java


I’m not sure I like this change. I tend to think that a class should not
expose public methods unless they are of immediate necessity. That helps
to reduce complexity and confusion for newcomers. If they prove to be
necessary they can always be added later on.

Also, in this case I don’t think any external object needs to know
whether the page masters for this page sequence were declared using
a single simple-page-master or a page-sequence-master. That knowledge
should be kept encapsulated inside the PageSequence class.

Finally, in the startOfNode method the get*Master method will be called
twice: once by has*Master to see if the master exists, and once to
actually get it. This is slightly overkill.

I think those new methods should at least be made private. Although the
original code simply looks fine to me...

WDYT?
Thanks,
Vincent