Summary: | advanced_notices.pl contains code obfuscation | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Architecture, internals, and plumbing | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | colin.campbell, f.demians, kyle, m.de.rooy, mtompset, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Attachments: |
Bug 13240: Remove some code obfuscation
Bug 13240: Remove commented warns Bug 13240: Remove some code obfuscation Bug 13240: Remove commented warns Bug 13240 [QA Followup] [PASSED QA] Bug 13240: Remove some code obfuscation [PASSED QA] Bug 13240: Remove commented warns [PASSED QA] Bug 13240 [QA Followup] |
Description
Jonathan Druart
2014-11-12 11:21:42 UTC
Created attachment 33478 [details] [review] 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. Created attachment 33479 [details] [review] Bug 13240: Remove commented warns 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. (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. (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 :) Created attachment 35361 [details] [review] 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> Created attachment 35362 [details] [review] Bug 13240: Remove commented warns Signed-off-by: Marcel 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) .. I would also prefer $a->{b}->{c}. I think it's more readable. Maybe we should vote on this for coding guidelines. (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 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 (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 (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}. Created attachment 35704 [details] [review] Bug 13240 [QA Followup] 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> 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> Created attachment 35707 [details] [review] [PASSED QA] Bug 13240 [QA Followup] Signed-off-by: Kyle M Hall <kyle@bywatersolutions.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 Patches pushed to master. Thanks Jonathan! |