[Koha-devel] Data Persistence and Plack

[Colin Campbell]

On 17/12/10 15:32, LAURENT Henri-Damien wrote:
> Le 17/12/2010 15:30, Colin Campbell a écrit :
>> On 17/12/10 11:34, LAURENT Henri-Damien wrote:
>>
>>> It seems that most of the Circular references in the code are owed to
>>> functions which have been created to do some checking before doing an
>>> operation...
>>> For instance :
>>> 	CanItemBeRenewed
>>> 	CanItemBeReserved
>>> 	CanBookBeReserved
>>> 	CanItemBeIssued
>>> 	CanItemBeIssued
>>> 	....
>>> 	Deletion of Serials in DelBiblio
>>>
>> Most of those routines are far too large and unmaintainable, many read
>> the same data repeatedly and all mix business logic and reading data. It
>> would probably be good if the "things" they deal with were abstracted
>> into proper objects that police their own destruction, it would also
>> give you the chance to have a more guaranteed interface to e.g. Item so
>> that we dont have to scatter validations about through the business
>> logic. Its one of the attractions of an ORM that it does this for you
>> but you don't need an ORM to do it. I think if you can abstract away
>> some of the current complexity it gets easier keep things clean.
>>
>> Colin
>>
> Thanks for this feedback.
> Nonetheless those functions are quite "generic" and if they need some
> refactoring, I guess that they would still need some external calls, for
> instance, you can Issue an item if it is not reserved...
> So those checks have to be done...
> Maybe it could be better abstracted... But I lack some hawkeye to tell
> how you would not need those external calls. I think that an exemple
> could be helpful in order to show me what could be done in your view.
The problem is that the Modules containing the external calls are large
and rambling. They export into our namespace lots we dont need. It is
very hard when calling those routines to know confidently what side
effects they may have.

So we get a
checkout( borrowercard, itemcard) {
  itemstuff = GetItem(itemcard)
blah ... blah...
  ItemXYZ(itemstuff->itemid);
blah .. blah ...
  ItemABC($var)

etc;
and we've got GetItem ItemXYZ ItemABC and loads more Item routines
imported. And we probably duplicate external accesses across them.

But if we have a wrapper class around item and do
checkout( borrowercard, itemcard) {
   my $item = Item->new(ittemcard);
blah ... blah ...
   $item->zyx();
blah .. blah
   $item->abc()

item is lexical, if necessary it can call its destructor when it goes
out of scope. We don't have to refetch data for different calls as it
persists while the variable persists and we've not imported a pile of
package variables into our namespace. Also in our business logic we can
avoid those GetItem then a long if else checking varying things and
replace that with a method call.

For the kind of cases you are mentioning we have simple boolean methods:
$item->renewable();

Cheers
Colin

-- 
Colin Campbell
Chief Software Engineer,
PTFS Europe Limited
Content Management and Library Solutions
+44 (0) 845 557 5634 (phone)
+44 (0) 7759 633626  (mobile)
colin.campbell at ptfs-europe.com
skype: colin_campbell2

http://www.ptfs-europe.com