[prev in list] [next in list] [prev in thread] [next in thread] 

List:       fop-dev
Subject:    Re: svn commit: r757852 - /xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/pagination/PageSequence.
From:       Adrian Cumiskey <dev () cumiskey ! com>
Date:       2009-03-25 12:06:40
Message-ID: 49CA1E50.6020000 () cumiskey ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]