On Mon, Sep 29, 2008 at 09:36:21AM -0400, Sébastien Nadeau wrote:

Hi Marc,

hello, If i paid attention to this code: - i used File::Spec::splitdir to create the directories using mkdir builtin. - i tested if the $authorityserverdir exists before the loop (no need to test the existence of all of them if the parent one does not exist) - i read the coding style to see what about $_. I'm a fan of it because i immediately read is as "the main variable of the loop". I only use it in little, unnested loops. I didn't: this was just an example to ask the koha people if such a cleanning work is welcome.

I hope I'm not looking too pretentious for my first post.

even if i disagree with you, it's good to have other points of view in mind. regards -- Marc Chantreux http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc

On Mon, Sep 29, 2008 at 10:26:28AM -0400, Sébastien Nadeau wrote:

Hi Marc,

I understand your point better now.

I think this is the kind of stylistic improvement that must be done.

But about the iterator variable, here's what Damian Conway says in his 'Perl Best Practices' great book:

Damian> It ($_) conveys nothing about the nature or purpose of the values it stores, sure! feel free to comment your code :) you can also use English; print $ARG; keeping the advantage of implicit syntaxes like ( /foo/ , -d, print ) when possible.

He then gives examples to show how things can go down rapidly. I tend to agree with that because as the code evolves, the inner content of the block may change and sooner or later an explicit iterator variable will be needed.

that is a better arguement for me: if you used the implicit syntax and you want to add a name for the old $_, you can (murphy will say *must*) forget to rewrite an instruction and this is just a new bug. regards -- Marc Chantreux http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc

On Mon, Sep 29, 2008 at 08:49:00AM -0500, Galen Charlton wrote:

for my $subdir (qw( lock register shadow tab key)) { ... }

i.e., please use an explicit loop variable instead of $_. Otherwise, I agree with rolling these up into a loop.

as i said to Sebastien: in this case, i think it add some readability. do you think that: for my $var ( list_generator ) { $var =~ /useless/; $var =~ s/old/new/; next unless -d $var; mkdir $_; } is more readable than: for ( list_generator ) { /useless/; s/old/new/; next unless -d; mkdir $_; } ? (it's a real question: i feel i can be wrong). I don't need to see the for line to see, for exemple, that the substitution applies on the current element of the list (and it's shorter to read and write). I think that be confortable with $_ (without abusing it) is a part of the perl programmer skills set. If you don't, you'll never use such usefull fonctions like grep and map. perl 5.10 makes $_ lexical and add the (_) prototype. It would be sad to not use it imho. can this practice discuted with all developpers and added in the coding style?

1. If you're doing this sort of stylistic change, please consider writing test cases.

you're right but i'm not fluent with it. Any doc about that (in the wiki or whatever?)

2. Unless you're revamping an entire module or script, don't mix cleanup (that theoretically shouldn't change behavior) with functionality changes in the same patch.

completely agreed: no idea to do it in another way.

3. If you fix a problem in one place, consider fixing it in all places.

all places in the file, right? doing it in every files of koha is a long job.

4. The goal of a refactoring should be to improve clarity, maintainability, testability, or logical structure. Please don't "refactor" just to show off your knowledge of Perl arcana; a newbie to Perl should have a fighting chance of understanding Koha's code.

don't worry: we feel the same about that. One of the reasons that leads me to a code cleaning is that i think that koha can be more attractive for contributors with a code that is more easier to understand. (that's why i'm for Template::Toolkit too)

A generic code-cleaning bug is too vague; I suggest opening a bug for each cleanup, to remain open until all instances of that cleanup have been completed.

i though about it but it will open a large range of bugs. the only one bug was just a proposal to limit the polution in bugs.koha.org. regards -- Marc Chantreux http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc

On Mon, Sep 29, 2008 at 11:03 AM, Galen Charlton <galen.charlton@liblime.com

wrote:

Hi,

On Mon, Sep 29, 2008 at 9:42 AM, Marc Chantreux <marc.chantreux@biblibre.com> wrote:

...

do you think that:

for my $var ( list_generator ) { $var =~ /useless/; $var =~ s/old/new/; next unless -d $var; mkdir $_; }

is more readable than:

for ( list_generator ) { /useless/; s/old/new/; next unless -d; mkdir $_; }

I do. :)

I don't. I actually prefer the latter. And if you construct the example more conventionally, it should be obvious what the topic is without any explanatory comment. foreach (@directories) { ... }

...

I think that be confortable with $_ (without abusing it) is a part of the perl programmer skills set. If you don't, you'll never use such usefull fonctions like grep and map.

I agree that a Perl programmer ought to know how to use $_. However, IMO the project needs to accept contributions from both expert and novice Perl programmers, and too much use of "punctuation" variables outside of the while (<FILE>) idiom reduces clarity.

I don't consider $_ at all an "advanced" construction. It's proper use should be common, certainly more common than other punctuation vars. On that note though, I don't see anything wrong with $! or $. either. I'd much prefer $. to $NR! Feel free to use $INPUT_LINE_NUMBER if you want, but that level of $VERBOSITY seems a bit too remedial for common usage. Case in point though, here I agree it is better to add a variable rather than assign to $_. But more broadly, coding to avoid $_ altogether is working against the language unnecessarily and, imho, unproductively. -Joe

On Mon, Sep 29, 2008 at 02:32:04PM -0400, Sébastien Nadeau wrote:

because we can do it. In nested constructs, unless you explicitly scope it using my $_, the brain will always need more efforts to determine on which $_ we are working.

that's what i said i don't use it in nested loops.

It is definatly simpler to use iterators in this case.

$_ isn't such one ? -- Marc Chantreux http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc

On Mon, Sep 29, 2008 at 04:15:23PM -0400, Sébastien Nadeau wrote:

...

$_ isn't such one ? I should have said: use explicit iterators.

that's the point i want to point in: i wonder if the problem is the use of $_ (that is just another variable name) or the use of implicity. regards -- Marc Chantreux http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc

Marc Chantreux wrote:

On Mon, Sep 29, 2008 at 12:43:52PM -0400, Joe Atzberger wrote:

...

Case in point though, here I agree it is better to add a variable rather than assign to $_. But more broadly, coding to avoid $_ altogether is working against the language unnecessarily and, imho, unproductively.

Joe++

For the moment, we have guys pro and 2 cons. Is it possible to poll and add it in the coding style?

I have to say I am not going in for heavy $_ use. Anytime we can avoid this, we should. Having implicit iterator is smart and looks bright. But it really has to be very carefully written AND documented. For instance if you are processing a string inside a loop of strings with a $_ and uses m// on an element, will the reader be able to know what should be in $_ after a m// is applied ? Sometimes, when we are debugging, things are getting tough because many ppl added many features in one loop that could/should have remained simple. I had to write many maps inside foreach, and the first time, I wanted to use foreach (@array_of_arrayrefs){ map{$_ }@$_; } Then use of $_ as iterator was rather a burden than a good thing. I donot think that using $_ will really ease the process. Maybe only with maps and greps, but in for/foreach/while loops, I donot see any good in this. My 2 cts. -- Henri-Damien LAURENT

On Mon, Sep 29, 2008 at 03:27:52PM -0500, Galen Charlton wrote:

We could, but it regardless of the decision, it would be of little practical effect unless it turns out people *really* care one way or the other. You know my opinion on the use of $_ as an implicit iterator, but I would not use that as a primary criterion for accepting or rejecting patches.

After a night of reflexion, i give up: I don't want another koha developper to loose some time to read code just because i gain some time to write it (i don't want to blame myself for it). Using perl stuff makes sense if everyone does it and even if it will be officially recommanded by the coding style, it will not be reality. $_ is not the only one point that can confuse the code reader so i also ban for my code: - grep, map - (List|Scalar)::Utils - the <instruction> if <test> statement; - the unless loop - regexp that using perl specific dialect and write my perl code as close as possible from php.

There are much bigger coding and stylistic problems to fix, including lack of use of the warnings pragma, circular module dependencies resulting in funky @EXPORT problems, inconsistent indentation, out-of-date, POD, low test suite coverage, and so on. I would rather that the style guide focus on the larger issues.

both are important because if a coder knows he have a chance to find the "koha way" in the coding style document, he'll use it. regards -- Marc Chantreux http://www.biblibre.com Expert en Logiciels Libres pour l'info-doc

Hi, On Tue, Sep 30, 2008 at 3:59 AM, MJ Ray <mjr@phonecoop.coop> wrote:

Marc Chantreux <marc.chantreux@biblibre.com> wrote:

...

$_ is not the only one point that can confuse the code reader so i also ban for my code: [...list of useful things...]

That would be a shame. I've seen at least one of the *Ms say that it won't be in the style guide either way (which makes sense - the main challenge at the moment is getting code written and fixed up), so use whichever feels easiest.

I agree. As I said, I would not use $_-as-implicit-iterator as a basis for rejecting patches, and I don't have an problem with people taking full advantage of Perl. I do prefer clarity, but since (obviously) there are differences of opinion about particular constructs, I think the coding style guide should focus on the essentials. Regards, Galen -- Galen Charlton VP, Research & Development, LibLime Koha 3.2 Release Manager galen.charlton@liblime.com p: 1-888-564-2457 x709 skype: gmcharlt

On Mon, Sep 29, 2008 at 12:43 PM, Joe Atzberger <ohiocore@gmail.com> wrote:

On Mon, Sep 29, 2008 at 11:03 AM, Galen Charlton <galen.charlton@liblime.com> wrote:

...

Hi,

On Mon, Sep 29, 2008 at 9:42 AM, Marc Chantreux <marc.chantreux@biblibre.com> wrote:

...

do you think that:

for my $var ( list_generator ) { $var =~ /useless/; $var =~ s/old/new/; next unless -d $var; mkdir $_; }

is more readable than:

for ( list_generator ) { /useless/; s/old/new/; next unless -d; mkdir $_; }

I do. :)

I don't. I actually prefer the latter. And if you construct the example more conventionally, it should be obvious what the topic is without any explanatory comment.

foreach (@directories) { ... }

This is a nice compromise for non-nested loops. But as far as that goes, its a tossup. However, avoiding unnecessary obfuscation should be the guiding rule, IMHO. So I think when it is clear, use punctuation strings and when it is not, don't. I only wonder if there is any performance gain by using $_ vs assigning a $var? Regards, Chris

Hi, On Mon, Sep 29, 2008 at 2:42 PM, Marc Chantreux <marc.chantreux@biblibre.com> wrote:

On Mon, Sep 29, 2008 at 10:03:19AM -0500, Galen Charlton wrote:

...

...

perl 5.10 makes $_ lexical and add the (_) prototype. It would be sad to not use it imho.

Which is fine, but we still need to support 5.8.

even in koha 3.x when x > 0 ?

Certainly for 3.2, and IMO, until such time as 5.8 is either deprecated, not significantly used, or is unable to support the features we want for Koha. Regards, Galen -- Galen Charlton VP, Research & Development, LibLime galen.charlton@liblime.com p: 1-888-564-2457 x709 skype: gmcharlt