[Koha-devel] [PATCH] C4::Table - A simple OO database abstraction layer.

[Joe Atzberger]

On Wed, Nov 5, 2008 at 5:42 AM, Paul POULAIN <paul.poulain at free.fr> wrote:

> Joe Atzberger a écrit :
> > Paul --
> >
> > I think you mean all the SQL should be in C4, and yes I agree with
> > that.  C4::Table does not receive any SQL as arguments to select, insert
> > or delete.  I.E., it preserves the encapsulation of SQL inside C4/*.
> >
> > This would speed up development of features because a C4/NewFeature.pm
> > module might not be necessary at all, or could focus on things special
> > to NewFeature besides basic table operations.  Once a table is added to
> > the DB, C4::Table can access it, no new code necessary.
> >
> > And the other code will be more recognizable, using the same kind of
> > $table->select({fieldname=>"value"}) statements.  Would be nice to be
> > able to use the same OO code accessing issues and old_issues, or any of
> > the XX and old_XX tables, for example.
>
> let me be more precise :
> do you agree that $table->select({fieldname => "value"}) is SQL
> somewhere : fieldname.


It is the table fieldname, yes.  But that is not SQL.   It would be the same
as if you were querying some object unrelated to a DB table.

If we change fieldname to newfieldname, with Koha as it's written now,
> you'll just have to dig C4 to find all occurrences of fieldname and
> update them.
>
> If you embeed $table->select... in a .pl, then that will be much more
> complex to maintain.


Not at all.  The current approach that most frequently returns a reference
to hashref (or reference to array of hashref) ALSO expects you to know the
fieldnames in each script retrieving the data.  This same argument style is
already present in functions like ModMember.

Perhaps it's useful to approach the question in reverse.  Say you have one
table and you to write a C4::Module to allow me to query that table.  If all
we ever needed was GetRowByPrimaryKey, it wouldn't be an issue.  But we know
we want to be able to search against any field.   How would you do it?

You could have:
use C4::Module;
my $row_a = GetRowByField1($field1);
my $row_b = GetRowByField2($field2);
my $row_c = GetRowByField3($field3);
# ... and so on ...

But that is obviously the least efficient and most duplicative approach.  If
you have n fields, you would need n different functions.  It also fails if
we want to specify matching against two fields.  It should be no surprise
that we don't solve that problem by making a whole bunch of specialized
combo functions like:
my $row_d = GetRowByField1xField3($field1, $field3);

That would only make matters worse, requiring additional functions for every
pairing of fields (equalling the sum 1 + 2 + 3 ... + n-1).  Very silly
indeed, since then we might want to specify 3, 4 or all n fields.  So we do
one big function:
my $row_e = GetRow($field1, $field2, $field3 ... );

This is representative of the approach taken by parts of Koha.  Although in
most cases Koha doesn't implement it, we would need to have n argument slots
for complete functionality.  We then have the problem of expressing when we
want the function to ignore an argument, and when we want to filter on a
value like zero.  And it starts to get ridiculous when you see calls like:
GetRow(undef, undef, undef, 3, undef, 0, "");

Sometimes the argument order matches the order of fields in the table, and
sometimes it doesn't.  I think having to remember the numerical position of
ALL n arguments is far worse than having to remember the names of just the
fields you care about.  In particular the problem of updating some fields in
a record, without updating ALL of them brings us to Koha's most advanced
argument style:
ModRow({id=>$id, field1=>$field1});  # or
GetRow({fieldname=>"value"});

But back to our hypothetical, say you were able to write the code to accept
a hashref and return the correct rows from the table.  It might look a bit
like the code in C4::Tags.  Paul, you seem to object to using the fieldnames
as the keys in the hashref, but nothing else would be more sensible.
There's no point to making me use a different label when the purpose really
is to match against a specific table field.  Certainly it is preferable to
the old style of:
my $member = GetMember($arg, 'cardnumber');

C4::Table is good enough to carp at me if I supply an unrecognized key.
Passing hashref arguments is best
practice<http://www.perl.com/pub/a/2006/02/23/advanced_subroutines.html>,
afaict <http://safari.oreilly.com/0596001738/perlbp-CHP-16-SECT-4>, and
allows clean conversion to OO style, where GetRow actually is more like a
constructor:
my $rowobj = C4::Module->new({fieldname=>"value"});

So imagining that you finish C4::Module and it works like we want... ok,
good!  After adding insert, modify and delete functions, you would still
have to write similar new code for every new DB table added.  C4::Table
gives it to you in one place, with error checking and internal caching of
structural data, for every existing Koha table and every Koha table that
*could* exist, using (in preliminary form) less than 250 lines of code!

I think that's about as many lines as we've now used in conversation about
it, and I admit I probably should put a lot of this into the module as
perldoc to make more apparent my motivation behind it.  Hopefully that is
more clear now, even if we may disagree about argument style.  If you know a
preferable alternative to hashref arguments, I would be glad to hear more
about it.

--Joe
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-devel/attachments/20081105/f5c2cc74/attachment-0003.htm>