[Koha-devel] Add rule for no subroutines in PL scripts

[Christopher Nighswonger]

On Wed, Sep 30, 2015 at 8:08 PM, David Cook <dcook at prosentient.com.au> wrote:
>Sorry, Chris, but I'm not sure I understand what you're saying here.

Let me see if I can clarify it a bit.

> I suppose I don't really think about it in terms of libraries but rather in terms of classes. In theory, a script should be able to be broken down into concepts which fit into classes.

In my understanding class implies a related (however loosely) set of
data and methods. So to put the suggestion under discussion into
practice one would need to essentially create a "module" for every
script which contained any "methods" unique to that script or create a
sort of catch-all module into which one would place methods which are
generally unrelated to each other, but do not properly belong to any
other module. Such a module would eventually become rather large and
unmanageable I think.

Take, for example, this script which produces the pdf file during a
label export.

http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=labels/label-create-pdf.pl;h=e69ba41e55634d5b15ad7a993edbd203b649424d;hb=HEAD

In it are two subs which might have been made methods of the
C4::Labels::Label class, but they are not properly members of that
class as they do not operate on a label object, but on the pdf object.
Perhaps they could have gone into C4::Creators::PDF class, but that is
really just a wrapper to make PDF::Reuse behave OO-like and those subs
are totally unrelated to PDF::Reuse. It might go into
C4::Creators::Lib as that is not properly a class, but a catch-all for
functions to permit them to be called OO-style, but all of the
functions in Lib are used by more than one script where the above
mentioned are used only once.

The script which produces the pdf file is not hard to debug/test as
the vast majority of it is OO. The very little bit which is not is
still rather straightforward. (BTW, are we talking about writing tests
for every single script as well as the various modules? Maybe I'm not
seeing something... which is a pretty common thing for me. :-)

>
> I think sometimes we end up using subroutines in scripts because we haven't scrutinized the actual "function" enough. For instance, take a look at tools/letter.pl.
>
> Another scary one is opac/oai.pl, although that's actually created packages in the pl script which I think is actually worse in a way.
>

I certainly agree with your observations in cases such as these. There
are undoubtedly many things in Koha which could and should be
refactored into OO form, and subs/functions are often used as an easy
out by authors. The labels (and friends) code was my feeble attempt to
do that in that section of the Koha world.

> Maybe it's not feasible to always have subroutines defined in Perl Modules. However, I rather there be a blanket ban on them, and allow exceptions to that rule on the discretion of the QA team and the RM.
>
> As Tomas has said, I think there's been an existing practice for some at least a few releases now to fail patches which have complex subroutines in Perl Files. This would simply be codifying an existing informal rule.

If it is already the practice of the QA team to examine each instance
and pass/fail on the merits of the particular form based on the
particular application, then it appears to me that there is no need
for a ban. What seems more reasonable is a formal statement of what
has been the practice: a case by case determination which requires the
author to justify cases that the QA team fails. A ban seems to imply a
complete unwillingness to consider the possibility of the potential
need of a sub/function. Although I'm sure that is not at all the
motive.

Ultimately, I'll yield to whatever everyone thinks is best. This is
just my humble (and probably uninformed) opinion.

Kind regards,
Chris