Hi I am continuing to work on Data Persistence and Plack support. In that matter, I am first focusing on obvious circular references between Modules. I first naively thought that simply using direct calls to functions without calling use module would solve the problem. But if compilation is OK then at runtime, it would fail unless all the modules are loaded in Plack... And in CGI mode, it would fail. 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 Those things would be solved if we would create new modules in order to perform those checks and then call them not in the module but in the script itself... A kind of Controller. My proposal is to create a new directory in C4/ called Decisions or Controller (I am not fixed on a namespace for that.) And then create inside one module for one "Object" Circulation Reserve, Biblio, Serials... And use that in the scripts. I am aware that it would not fix circular references in the data structures itself... But that would be a first step towards persistence. Then we have to inspect at runtime the increase in Memory consuming and control with introspection... Maybe a server could be built and some apache logs replayed on that so that memory consumption could be traced. If you are working on the same stuff, and want to share what point you are at... Then come on #koha channel or let's organise a meeting around that. Any feedback would be welcome. -- Henri-Damien LAURENT
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 -- 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@ptfs-europe.com skype: colin_campbell2 http://www.ptfs-europe.com
Le 17/12/2010 15:30, Colin Campbell a écrit :
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. Hi Colin : so... who's first : the egg or the chicken ? (frenchism suspected)
Because an ORM can't be achieved without persistency (or we will get awful response times...) I'm in favor of doing one step after the other, and ORM is the step #2, after persistancy that is the #1. IMO, if you disagree, pls argue & convince me. -- Paul POULAIN http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc Tel : (33) 4 91 81 35 08
On 17/12/10 14:37, Paul Poulain wrote:
Le 17/12/2010 15:30, Colin Campbell a écrit :
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. Hi Colin : so... who's first : the egg or the chicken ? (frenchism suspected)
Because an ORM can't be achieved without persistency (or we will get awful response times...)
I'm in favor of doing one step after the other, and ORM is the step #2, after persistancy that is the #1. IMO, if you disagree, pls argue & convince me.
Many people implement persistency by using an ORM but if you look at what I wrote I said we did not need an ORM to achieve this. C. -- 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@ptfs-europe.com skype: colin_campbell2 http://www.ptfs-europe.com
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. -- Henri-Damien LAURENT
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@ptfs-europe.com skype: colin_campbell2 http://www.ptfs-europe.com
The problem is that the Modules containing the external calls are large and rambling. They export into our namespace lots we dont need. Ok... But it is what we have at the moment. And we cannot rewrite all Koha at once... :D Maybe we could try and use more EXPORT_OK and then create tags for actions (circulation, etc..) and try and select the tags we want when we "USE" a module. This could be a step, but would not be enough reading
Le 17/12/2010 18:00, Colin Campbell a écrit : the rest of your message. So ... Let's try and see more.
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()
mmmm... I think I get you. Would need even deeper refactoring then... Could we not get step by step ? Or maybe there is some background or easy changes I miss.
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();
Well, still renewable would have to check reserves.... and therefore would use a reserve object in items... (And an item is not renewable per se... It is an issue which can or cannot (impossible) or maynot (you need confirmation) be renewed depending on circumstances. But I got your idea) I wanted to break circular dependencies between Reserves and Circulation and Reserves or Reserves and Items (see the picture brought along). Maybe it is only with object definition and paradygm that we could solve this out. But what occurs to me was that it might have been quite easy to split true atomic object methods (Add, Edit, delete, search) and the definition of ability to do a function (would it be Decision Table driven) that might take into account the whole environment of an object... For instance : Say I want to Delete a Biblio. Is it linked to a subscription, to an order, to an issue ? Then donot delete that... Or delete all the related objects... Or ask confirmation... I think it is good that it is a method. But it might be misplaced in C4::Biblio. Since makes use of the global environment. And therefore makes intensive usage of use C4:: ... This is why I wanted to confine those types of functions into a kind of "Controller" object... This was what I realized with the picture and code analysis... This was my reflection, and what I proposed to work on and have feedback on. I appreciate your feedback, and hope though that with Data Persistence, we will be able to get Koha up to better and widely used coding practises and standards of PERL (using Moose, and DBIx::Class or Schema, maybe KiokuDB and Modern::Perl), and really ease the QA job eventually. Since Data Persistence is one of the battles for 3.4 http://wiki.koha-community.org/wiki/RFCs_for_Koha_3.4 , let's join forces in that battle, win that one, all together... Template::Toolkit is really nicely coped by Catalyst. Hopes that helps. -- Henri-Damien LAURENT
On 17/12/10 18:09, LAURENT Henri-Damien wrote:
Le 17/12/2010 18:00, Colin Campbell a écrit :
Maybe we could try and use more EXPORT_OK and then create tags for actions (circulation, etc..) and try and select the tags we want when we "USE" a module.
Yes if you are creating a new interface in C4 don't please stick everything in @EXPORT. In fact if you create a new C4 module take a couple of minutes to read perldoc Exporter rather than just copying the style of existing modules. 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@ptfs-europe.com skype: colin_campbell2 http://www.ptfs-europe.com
participants (4)
-
Colin Campbell -
LAURENT Henri-Damien -
LAURENT Henri-Damien -
Paul Poulain