[Liblas-devel] Design/architecture questions

[Howard Butler]

On Dec 4, 2010, at 10:25 AM, Mateusz Loskot wrote:

> On 01/12/10 18:10, Howard Butler wrote:
>> A few questions arise:
>> 
>> * Instead of setting filters and transforms on the 
>> liblas::Reader/liblas::Writer (and caching these lists as data 
>> members on either), what tact should we take?
> 
> liblas::Reader is a thin handle giving access to the actual reader
> operating on LAS data. It does not provide any detailed logic itself,
> it's just a proxy object to which a specific reader implementation
> is attached.
> 
> liblas::ReaderI is the actual definition reader interface and behaviour.
> Whatever a LAS reader is supposed to do, it should be outlined in ReaderI.
> 
> Next, there are implementations of the ReaderI and these implementations
> can be attached to liblas::Reader proxy depending
> on various circumstances.
> 
> Given that, liblas::ReaderI is the place where transformation, filtering
> and all the other behaviour should be specified.
> 
> How the transformers or filters are handled? It should be up to
> particular implementation of ReaderI.
> Some may cache it, some may not, but I don't think ReaderI should cache
> anything here.
> 
> It may feels that particular implementations of ReaderI will have
> repeated members caching transformers and filters, but caching them in
> ReaderI makes more hassle than it saves on typing.
> What if I want to have reader implemented in such way that transformers
> are cached in some special sorted order?
> 

I think I understand.  A CachedReaderImpl, which currently implements ReaderI, will have some SetTransforms and SetFilters methods that it needs to implement, and it can choose whether or not to do anything with them.  I agree this is much cleaner than hanging them off of the liblas::Reader.

>> * How do we support injecting a custom header into the pimpl?  Right
>> now, every method on the pimpl takes an explicit header.  We know the
>> custom header at construction time, and we are currently caching it
>> on the Reader. Should this be moved down into the pimpl to be
>> cached?
> 
> Again, IMHO, this is part of ReaderI.
> If header is a part of object state, it should be passed in ReaderI ctor
> (factory). If reader operations are independent in such way they can use
> different headers, then pass header as parameter of those operations.
> The ReaderI is an interface and it does not care how specific
> implementors deal with header, they can cache it or not.

After mucking with these internals for the better part of a year, it's pretty clear to me that the header should very much be part of the ReaderI's constructor.  As they are now, readers end up caching, comparing and syncing passed-in readers to protect from them changing.  If we have a single header passed in at construction time, this silliness can go away.

> 
> I'm answering the questions and discussing my ideas of how I see it.
> I'm not referring current state, how changes may affect users, etc.

I think the impact of this refactoring would be rather small at this point.  I only know of one custom reader that exists outside of those in libLAS.

> 
> Disclaimer: I'm strongly against technique called "implementation
> inheritance".

AFAIK, there is one case of it in the reader right now... ReaderImpl and CachedReaderImpl, but implementation inheritance is not our public prescription for having people reuse libLAS code.


> Code reuse based on implementation inheritance is a myth
> and makes more troubles than it saves on typing.
> The implementation inheritance is OK as long as it's private
> and does not make use of substitutability, but this is not the case in
> ReaderI.

Thank you very much for the discussion and feedback.  I will continue to weed the garden in the next week or so until we can get to a thin-handle liblas::Reader object and be able to write a factory to produce them.

This refactoring is going to bring a small regression for any C++ users who might have been using libLAS 1.2's liblas::Reader::SetInputSRS and friends.  That stuff clearly needs to go away.  We have generic TransformI's now that implement this single, special case.  I'd be curious to hear of the impact this change would have on anyone other than myself.

Howard