Bug 13240 - advanced_notices.pl contains code obfuscation
Summary: advanced_notices.pl contains code obfuscation
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-12 11:21 UTC by Jonathan Druart
Modified: 2015-12-03 22:11 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13240: Remove some code obfuscation (2.27 KB, patch)
2014-11-12 11:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13240: Remove commented warns (3.29 KB, patch)
2014-11-12 11:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13240: Remove some code obfuscation (2.37 KB, patch)
2015-01-19 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13240: Remove commented warns (3.41 KB, patch)
2015-01-19 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13240 [QA Followup] (1.82 KB, patch)
2015-02-06 14:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13240: Remove some code obfuscation (2.40 KB, patch)
2015-02-06 14:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13240: Remove commented warns (3.43 KB, patch)
2015-02-06 14:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13240 [QA Followup] (1.89 KB, patch)
2015-02-06 14:48 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-11-12 11:21:42 UTC
my $bar;
my $foo = $bar->{borrowernumber} ||= {};
$foo->{bar} ||= 'something';
$foo->{two}++;

What does $bar contain?
Comment 1 Jonathan Druart 2014-11-12 11:29:08 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-11-12 11:29:12 UTC Comment hidden (obsolete)
Comment 3 Frédéric Demians 2014-11-13 13:08:49 UTC
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.
Comment 4 M. Tompsett 2014-11-19 17:45:53 UTC
(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.
Comment 5 Jonathan Druart 2014-11-20 08:15:58 UTC
(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 :)
Comment 6 Marcel de Rooy 2015-01-19 08:16:25 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2015-01-19 08:16:29 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2015-01-19 08:20:40 UTC
(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) ..
Comment 9 Kyle M Hall 2015-01-23 16:34:11 UTC
I would also prefer $a->{b}->{c}. I think it's more readable.

Maybe we should vote on this for coding guidelines.
Comment 10 M. Tompsett 2015-01-23 17:54:16 UTC
(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#Agenda
Comment 11 Colin Campbell 2015-02-06 09:52:05 UTC
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
Comment 12 Jonathan Druart 2015-02-06 11:29:50 UTC
(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
Comment 13 M. Tompsett 2015-02-06 13:41:44 UTC
(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}.
Comment 14 Kyle M Hall 2015-02-06 14:45:42 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-02-06 14:48:01 UTC
Created attachment 35705 [details] [review]
[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>
Comment 16 Kyle M Hall 2015-02-06 14:48:08 UTC
Created attachment 35706 [details] [review]
[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>
Comment 17 Kyle M Hall 2015-02-06 14:48:11 UTC
Created attachment 35707 [details] [review]
[PASSED QA] Bug 13240 [QA Followup]

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Colin Campbell 2015-02-06 15:08:46 UTC
(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
Comment 19 Tomás Cohen Arazi 2015-02-10 17:10:32 UTC
Patches pushed to master.

Thanks Jonathan!