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

[Vincent Hennebert <vh...@gmail.com>]

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
<snip/>

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

[Adrian Cumiskey <de...@cumiskey.com>]

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
>>     
> <snip/>
>
> 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
>
>