[Bug 13240] New: advanced_notices.pl contains code obfuscation
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Bug ID: 13240 Summary: advanced_notices.pl contains code obfuscation Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: jonathan.druart@biblibre.com QA Contact: testopia@bugs.koha-community.org my $bar; my $foo = $bar->{borrowernumber} ||= {}; $foo->{bar} ||= 'something'; $foo->{two}++; What does $bar contain? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|gmcharlt@gmail.com |jonathan.druart@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #1 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33478 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33478&action=edit Bug 13240: Remove some code obfuscation my $bar; my $foo = $bar->{borrowernumber} ||= {}; $foo->{one} ||= 'something'; $foo->{two}++; What does $bar contain? $VAR1 = { 'borrowernumber' => { 'two' => 1, 'bar' => 'something' } }; Not really obvious. Maybe something I did not see is hidden. Test plan: Verify the digest for DUE and PREDUE work as before. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #2 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 33479 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=33479&action=edit Bug 13240: Remove commented warns -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Frédéric Demians <frederic@tamil.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |frederic@tamil.fr --- Comment #3 from Frédéric Demians <frederic@tamil.fr> --- You replace: my $digest = $due_digest->{$upcoming->{'borrowernumber'}} ||= {}; $digest->{email} ||= $from_address; $digest->{count}++; With: $due_digest->{$upcoming->{borrowernumber}}{email} = $from_address; $due_digest->{$upcoming->{borrowernumber}}{count}++; But why exactly? The first syntax is Perlish, with ||=, but not to the point to be unreadable. And the second syntax uses autovivification which isn't that readable either. And the second syntax uses $h->{}{} which isn't so good: $h->{}->{} should be preferred, imho. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Frédéric Demians from comment #3)
You replace:
my $digest = $due_digest->{$upcoming->{'borrowernumber'}} ||= {}; $digest->{email} ||= $from_address; $digest->{count}++;
With:
$due_digest->{$upcoming->{borrowernumber}}{email} = $from_address; $due_digest->{$upcoming->{borrowernumber}}{count}++;
But why exactly?
Why create variables when you don't need to? Are we so desperate for speed that much that we want to lose simple readability?
The first syntax is Perlish, with ||=, but not to the point to be unreadable.
To those unfamiliar with Perl "pointers" (for a lack of a better word, since it is a NEW syntax to me), it is unreadable. Once you realize that $digest "points" to what $due_digest->{$upcoming->{'borrowernumber'}} points to, then it does make sense.
And the second syntax uses autovivification which isn't that readable either.
Actually, autovivification is far more readable and understandable than using a secondary pointer to fill in another data structure.
And the second syntax uses $h->{}{} which isn't so good: $h->{}->{} should be preferred, imho.
I agree with Frédéric Demians on this, though only because it makes the change more apparently equal to me, not because it actually generates a better or different data structure. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #5 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to M. Tompsett from comment #4)
And the second syntax uses $h->{}{} which isn't so good: $h->{}->{} should be preferred, imho.
I agree with Frédéric Demians on this, though only because it makes the change more apparently equal to me, not because it actually generates a better or different data structure.
I have always used this syntax (4 years), and I won't change now :) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #6 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 35361 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35361&action=edit Bug 13240: Remove some code obfuscation my $bar; my $foo = $bar->{borrowernumber} ||= {}; $foo->{one} ||= 'something'; $foo->{two}++; What does $bar contain? $VAR1 = { 'borrowernumber' => { 'two' => 1, 'bar' => 'something' } }; Not really obvious. Maybe something I did not see is hidden. Test plan: Verify the digest for DUE and PREDUE work as before. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33479|0 |1 is obsolete| | --- Comment #7 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 35362 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35362&action=edit Bug 13240: Remove commented warns Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #8 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #5)
(In reply to M. Tompsett from comment #4)
And the second syntax uses $h->{}{} which isn't so good: $h->{}->{} should be preferred, imho.
I agree with Frédéric Demians on this, though only because it makes the change more apparently equal to me, not because it actually generates a better or different data structure.
I have always used this syntax (4 years), and I won't change now :)
Although $a->{b}->{c} is more readable, it seems that both variations are both widespread in Koha. Changing a habit can be a good thing (at least sometimes) .. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- I would also prefer $a->{b}->{c}. I think it's more readable. Maybe we should vote on this for coding guidelines. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #10 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Kyle M Hall from comment #9)
Maybe we should vote on this for coding guidelines.
I added it as an agenda item to http://wiki.koha-community.org/wiki/Development_IRC_meeting_4_February_2015#... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |colin.campbell@ptfs-europe. | |com --- Comment #11 from Colin Campbell <colin.campbell@ptfs-europe.com> --- In the perl world using $x->{one}->{two} rather than $x->{one}{two} is the most common. one reason is that the arrow syntax can be used in a string e.g. print "Var: $x->{one}->{two}" using the non-arrow syntax in this way does not do what you may think it does Plus when you get to multiple levels of indirection the arrow-less version gets more ambigious -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #12 from Jonathan Druart <jonathan.druart@biblibre.com> --- (In reply to Colin Campbell from comment #11)
In the perl world using $x->{one}->{two} rather than $x->{one}{two} is the most common. one reason is that the arrow syntax can be used in a string e.g. print "Var: $x->{one}->{two}"
using the non-arrow syntax in this way does not do what you may think it does
Are you sure? I often use it, use Modern::Perl; my $h = { foo => { bar => 'foobar' } }; say "my string with $h->{foo}->{bar}"; say "my string with $h->{foo}{bar}"; outputs: my string with foobar my string with foobar -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #13 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Jonathan Druart from comment #12) [SNIP]
I often use it,
use Modern::Perl; my $h = { foo => { bar => 'foobar' } }; say "my string with $h->{foo}->{bar}"; say "my string with $h->{foo}{bar}";
outputs:
my string with foobar my string with foobar
I wrote my own code to see the same thing. Which is why I said, "I agree with Frédéric Demians on this, though only because it makes the change more apparently equal to me, not because it actually generates a better or different data structure." back in comment #4. Structurally, it seems identical, but syntax-wise, I still prefer the $h->{foo}->{bar}. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #33478|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35704 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35704&action=edit Bug 13240 [QA Followup] -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35361|0 |1 is obsolete| | Attachment #35362|0 |1 is obsolete| | Attachment #35704|0 |1 is obsolete| | --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35705 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35705&action=edit [PASSED QA] Bug 13240: Remove some code obfuscation my $bar; my $foo = $bar->{borrowernumber} ||= {}; $foo->{one} ||= 'something'; $foo->{two}++; What does $bar contain? $VAR1 = { 'borrowernumber' => { 'two' => 1, 'bar' => 'something' } }; Not really obvious. Maybe something I did not see is hidden. Test plan: Verify the digest for DUE and PREDUE work as before. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #16 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35706 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35706&action=edit [PASSED QA] Bug 13240: Remove commented warns Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 35707 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35707&action=edit [PASSED QA] Bug 13240 [QA Followup] Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 --- Comment #18 from Colin Campbell <colin.campbell@ptfs-europe.com> --- (In reply to Jonathan Druart from comment #12)
(In reply to Colin Campbell from comment #11)
In the perl world using $x->{one}->{two} rather than $x->{one}{two} is the most common. one reason is that the arrow syntax can be used in a string e.g. print "Var: $x->{one}->{two}"
using the non-arrow syntax in this way does not do what you may think it does
Are you sure? I often use it,
use Modern::Perl; my $h = { foo => { bar => 'foobar' } }; say "my string with $h->{foo}->{bar}"; say "my string with $h->{foo}{bar}";
outputs:
my string with foobar my string with foobar
Without pausing fpr thought its not obvious which of the first three is doing what you want: #!/usr/bin/perl use feature qw( say ); my $list = [ 'foo', 'bar' ]; say "my string with $list[0]"; say "my string with ${list}[0]"; say "my string with ${$list}[0]"; say "my string with $list->[0]"; outputs: my string with my string with ARRAY(0x936cb8)[0] my string with foo my string with foo -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13240 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Pushed to Master --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks Jonathan! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org