Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > Perl > Perl Misc > Re: More idiomatic

Reply
Thread Tools

Re: More idiomatic

 
 
J. Gleixner
Guest
Posts: n/a
 
      04-15-2013
On 04/15/13 13:17, David Harmon wrote:
> Here is some code I wrote in another newsgroup. It is probably
> obvious that it was written by an old C hacker. If anybody would
> like to suggest how it could be better perl or more idiomatic,
> go ahead.
>
>>>>

>
> use strict; use warnings;
> open my $fin, 't1.txt';


Use 3-argument open handle failure (die) if it can't open.
perldoc -f open

> my $fromline = '';
> my $useragent = '';
> my $hascont = 'n';
> my $crosspost = 0;
> my %statstab;



>
> while (<$fin>) {
> chomp;
> if (/Newsgroups: .*,/i) {
> $crosspost = 1;
> next;
> }


> if (/^Content-Type:/i) {
> $hascont = 'y';
> next;
> }


Even a C Hacker should have learned about using else...else if...

Use elsif on the rest of these, since it can't start with more than one
of these.. and avoids the need of 'next;' in every if. If none of these
could also have 'Newsgroups: ' in it, then all of these, except for the
first if, should be elsif's.

perldoc perlintro

> if (/^From: /i) {
> $fromline = $_;
> $fromline =~ s/<.*>/<...>/;
> next;
> }
> if (/^User-Agent^X-News-Software^X-Newsreader:/i) {

/^(?:User-Agent|X-News-Software|X-Newsreader):/i
Tiny bit shorter..

> $useragent = $_;
> $useragent =~ s/ \(.*//;
> $useragent =~ s/^.*:\s*//;
> next;
> }
> if (/^$/) {
> if ($fromline ne '' and not $crosspost) {
> ++$statstab{$fromline.'##'.$useragent.'##'.$hascon t};
> }
> $fromline = $useragent = '';
> $crosspost = 0; $hascont = 'n';
> }
> }

close( $fin );
>
> foreach (sort keys %statstab) {

next unless statstab{$_} > 1;

> if ( $statstab{$_}> 1) {
> ($fromline,$useragent,$hascont) = split '##';


Since they're used specifically as a result of split, in
this loop, then define them ('my') in this scope instead
of re-using the global variables. It works, in this case,
however it's better to narrow the scope of the variables.

> printf("%s %-24s %3d %s\n",
> $hascont, $useragent, $statstab{$_}, $fromline);
> }
> }


 
Reply With Quote
 
 
 
 
Rainer Weikusat
Guest
Posts: n/a
 
      04-15-2013
"J. Gleixner" <(E-Mail Removed)> writes:
> On 04/15/13 13:17, David Harmon wrote:


[...]

>> while (<$fin>) {
>> chomp;
>> if (/Newsgroups: .*,/i) {
>> $crosspost = 1;
>> next;
>> }

>
>> if (/^Content-Type:/i) {
>> $hascont = 'y';
>> next;
>> }

>
> Even a C Hacker should have learned about using else...else if...


This doesn't make much sense here because there is no common code
after the sequence of if-statements. The 'next' communicates this
clearly. Using an if - elsif - elsif - else cascade instead would
server to obfuscate this fact instead.

[...]

>> foreach (sort keys %statstab) {

> next unless statstab{$_} > 1;
>
>> if ( $statstab{$_}> 1) {
>> ($fromline,$useragent,$hascont) = split '##';

>
> Since they're used specifically as a result of split, in
> this loop, then define them ('my') in this scope instead
> of re-using the global variables. It works, in this case,
> however it's better to narrow the scope of the variables.


It is not 'better' to declare identically named variables in bazillion
inner scopes except if one if optimizing for 'maximal outsider
confusion' aka 'It was hard to write. It should be hard to read."

 
Reply With Quote
 
 
 
 
C.DeRykus
Guest
Posts: n/a
 
      04-15-2013
On Monday, April 15, 2013 12:24:25 PM UTC-7, Rainer Weikusat wrote:
> "J. Gleixner" <(E-Mail Removed)> writes:
>
> > On 04/15/13 13:17, David Harmon wrote:

>
>
>
> [...]
>
>
>
> >> while (<$fin>) {

>
> >> chomp;

>
> >> if (/Newsgroups: .*,/i) {

>
> >> $crosspost = 1;

>
> >> next;

>
> >> }

>
> >

>
> >> if (/^Content-Type:/i) {

>
> >> $hascont = 'y';

>
> >> next;

>
> >> }

>
> >

>
> > Even a C Hacker should have learned about using else...else if...

>
>
>
> This doesn't make much sense here because there is no common code
>
> after the sequence of if-statements. The 'next' communicates this
>
> clearly. Using an if - elsif - elsif - else cascade instead would
>
> server to obfuscate this fact instead.
>
>


But similarly you could argue "next" is confusing
because it suggests that you need to bypass common
code at the end of the conditionals. Since there
isn't any common code, "next" is unnecessary and
adds line noise. Of course, if it's a huge sequence
of conditionals, you could argue that the added
clarity trumps line noise.

> ...


--
Charles DeRykus
 
Reply With Quote
 
Rainer Weikusat
Guest
Posts: n/a
 
      04-15-2013
"C.DeRykus" <(E-Mail Removed)> writes:
> On Monday, April 15, 2013 12:24:25 PM UTC-7, Rainer Weikusat wrote:
>> "J. Gleixner" <(E-Mail Removed)> writes:
>> > On 04/15/13 13:17, David Harmon wrote:


[...]


>> >> if (/Newsgroups: .*,/i) {
>> >> $crosspost = 1;
>> >> next;
>> >> }


[...]

>> > Even a C Hacker should have learned about using else...else if...

>>
>> This doesn't make much sense here because there is no common code
>> after the sequence of if-statements. The 'next' communicates this
>> clearly. Using an if - elsif - elsif - else cascade instead would
>> server to obfuscate this fact instead.

>
> But similarly you could argue "next" is confusing
> because it suggests that you need to bypass common
> code at the end of the conditionals.


There is 'common code' after each if block, namely, all the remaining if
conditions which have no special relation to each other and 'next' has
no special relation to them.
 
Reply With Quote
 
C.DeRykus
Guest
Posts: n/a
 
      04-15-2013
On Monday, April 15, 2013 1:59:30 PM UTC-7, Rainer Weikusat wrote:
> "C.DeRykus" <(E-Mail Removed)> writes:
>
> > On Monday, April 15, 2013 12:24:25 PM UTC-7, Rainer Weikusat wrote:

>
> >> "J. Gleixner" <(E-Mail Removed)> writes:

>
> >> > On 04/15/13 13:17, David Harmon wrote:

>
>
>
> [...]
>
>
>
>
>
> >> >> if (/Newsgroups: .*,/i) {

>
> >> >> $crosspost = 1;

>
> >> >> next;

>
> >> >> }

>
>
>
> [...]
>
>
>
> >> > Even a C Hacker should have learned about using else...else if...

>
> >>

>
> >> This doesn't make much sense here because there is no common code

>
> >> after the sequence of if-statements. The 'next' communicates this

>
> >> clearly. Using an if - elsif - elsif - else cascade instead would

>
> >> server to obfuscate this fact instead.

>
> >

>
> > But similarly you could argue "next" is confusing

>
> > because it suggests that you need to bypass common

>
> > code at the end of the conditionals.

>
>
>
> There is 'common code' after each if block, namely, all the remaining if
>
> conditions which have no special relation to each other and 'next' has
>
> no special relation to them.



But, there really didn't need to be any common code
since each "if" clause was accompanied with a "next".

So there's no compelling reason to not code this as
"if-elsif..." and avoid the noisy "next" insertions.

--
Charles DeRykus
 
Reply With Quote
 
Rainer Weikusat
Guest
Posts: n/a
 
      04-15-2013
"C.DeRykus" <(E-Mail Removed)> writes:
> On Monday, April 15, 2013 1:59:30 PM UTC-7, Rainer Weikusat wrote:


[...]

>> There is 'common code' after each if block, namely, all the remaining if
>> conditions which have no special relation to each other and 'next' has
>> no special relation to them.

>
> But, there really didn't need to be any common code
> since each "if" clause was accompanied with a "next".
>
> So there's no compelling reason to not code this as
> "if-elsif..." and avoid the noisy "next" insertions.


The compelling reason is that execution continues with the next
statement after the if-elsif... cascade in case of such a cascade. But
when there is not such 'next statement', the whole construct is just
a blind: It forces someone who is not familiar with the code to go
looking for such common code only to find none.
 
Reply With Quote
 
J. Gleixner
Guest
Posts: n/a
 
      04-15-2013
On 04/15/13 14:55, David Harmon wrote:
> On Mon, 15 Apr 2013 14:04:32 -0500 in comp.lang.perl.misc, "J.
> Gleixner"<(E-Mail Removed)> wrote,

[...]
>> Use elsif on the rest of these, since it can't start with more than one
>> of these.. and avoids the need of 'next;' in every if.

>
> In my opinion 'next;' is better in this case because 'else if'
> requires the reader to look below to see if anything else may happen
> before the end of the loop.


You asked for "idiomatic" suggestions. I rarely see code that follows
your "opinion", but it's not code I'll have to fix, so do whatever
you want.

There are times to use if/next, however using it in every
if block is not idiomatic.

>
>>> if (/^User-Agent^X-News-Software^X-Newsreader:/i) {

>> /^(?:User-Agent|X-News-Software|X-Newsreader):/i
>> Tiny bit shorter..

>
> Actually, it is *exactly* the same number of characters.
> And requires remembering (?:


Knowing (? is idiomatic to Perl.

It was more for showing how you could avoid repetition... Three
carats, three colons... If you need to add another one, it won't
require those common characters, and it's clear that whatever it is
you are looking for starts from the beginning.. If the match
can be from the beginning, or at any place, then it might be
coded differently, but it this case they all start from the
beginning, so do it once.

>
> What with the added grouping, does it make the regex any more
> efficient?


No grouping.

>
>>> }

>> close( $fin );

>
> Does that matter?


Again, it's idiomatic.. For ever open, there should be a close.
That's common in most languages. Or 'use autodie;'.

>
>>> foreach (sort keys %statstab) {

>> next unless statstab{$_}> 1;

>
> Here you give me the opposite advice regarding 'next;' as you did
> above.


Not at all related.

There are a lot of times to use 'next', and this is one of many.

You'd probably see this from a person who is well versed in Perl
and you'd see the if() with folks who aren't as comfortable, which
again would be idiomatic.
 
Reply With Quote
 
C.DeRykus
Guest
Posts: n/a
 
      04-15-2013
On Monday, April 15, 2013 2:24:17 PM UTC-7, Rainer Weikusat wrote:
> "C.DeRykus" <(E-Mail Removed)> writes:
>
> > On Monday, April 15, 2013 1:59:30 PM UTC-7, Rainer Weikusat wrote:

>
>
>
> [...]
>
>
>
> >> There is 'common code' after each if block, namely, all the remaining if

>
> >> conditions which have no special relation to each other and 'next' has

>
> >> no special relation to them.

>
> >

>
> > But, there really didn't need to be any common code

>
> > since each "if" clause was accompanied with a "next".

>
> >

>
> > So there's no compelling reason to not code this as

>
> > "if-elsif..." and avoid the noisy "next" insertions.

>
>
>
> The compelling reason is that execution continues with the next
>
> statement after the if-elsif... cascade in case of such a cascade. But
>
> when there is not such 'next statement', the whole construct is just
>
> a blind: It forces someone who is not familiar with the code to go
>
> looking for such common code only to find none.


And the multiple "if...next" code forces you to read
through them all anyway to determine if there's common
code as well. After all, at some point, an "if..." may
not have a "next". You'll be schleppen through lots of
noisy "next" statements to ensure that's the case.

There's no "One True Way". Stylistically, I'd prefer
the "if...elsif" as the more idiomatic way but TIMTODI
is the "next" best option.

--
Charles DeRykus


 
Reply With Quote
 
Rainer Weikusat
Guest
Posts: n/a
 
      04-16-2013
"J. Gleixner" <(E-Mail Removed)> writes:
> On 04/15/13 14:55, David Harmon wrote:
>> On Mon, 15 Apr 2013 14:04:32 -0500 in comp.lang.perl.misc, "J.
>> Gleixner"<(E-Mail Removed)> wrote,


[...]

>>>> }
>>> close( $fin );

>>
>> Does that matter?

>
> Again, it's idiomatic..


According to

http://oald8.oxfordlearnersdictionar...nary/idiomatic

'idiomatic' means

containing expressions that are natural to a native speaker of
a language

It is somewhat unclear what this is supposed to mean in the context of
a programming language but it certainly doesn't mean "include useless
code because 'everyone else does it'". Perl supports proper stack
unwinding and because of this, explicitly closing filehandles stored
in lexical variables is never necessary. The perl runtime will also
close all filehandles before the script exists.
 
Reply With Quote
 
Rainer Weikusat
Guest
Posts: n/a
 
      04-16-2013
"C.DeRykus" <(E-Mail Removed)> writes:
> On Monday, April 15, 2013 2:24:17 PM UTC-7, Rainer Weikusat wrote:
>> "C.DeRykus" <(E-Mail Removed)> writes:


[...]

>> The compelling reason is that execution continues with the next
>> statement after the if-elsif... cascade in case of such a cascade. But
>> when there is not such 'next statement', the whole construct is just
>> a blind: It forces someone who is not familiar with the code to go
>> looking for such common code only to find none.

>
> And the multiple "if...next" code forces you to read
> through them all anyway to determine if there's common
> code as well. After all, at some point, an "if..." may
> not have a "next". You'll be schleppen through lots of
> noisy "next" statements to ensure that's the case.


I've meanwhile managed to understand what you were writing about:
Considering the way the posted code was written, the only 'thing' the
next statements actually skipped where the subsequent if tests and the
only reason why 'subsequent if tests' exist at all on this code path
is because if - elsif ... else wasn't used.

But that's IMHO besides the point: The terminating next marks each
if-block as 'self-contained' similar to the way the 'break' statement
works in a switch(..) { } in C: For this case, no further code within
the current block will be executed. And this is different from if
..... which denotes a set of alternate 'sibling sub-blocks' with the
current block. One could visualize this as something like

------
| stmt |
------

------
| stmt |
------

------- ------- -------
| if | | elsif | | else |
------- ------- -------

------
| stmt |
------

There's a 'point' after the if .. -cascade where all the alternate
codepaths end up and if there is nothing at this point, the if ... is
misleading.

> There's no "One True Way".


It is possible to argue about objective properties of the various
alternatives and to form a reasoned opinion based on that.
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
more idiomatic way to avoid errors when calling method on variable that may be nil? Charles Calvert Ruby 33 10-22-2010 11:44 AM
Question about idiomatic use of _ and private stuff. Steven W. Orr Python 4 02-27-2007 08:59 PM
[ANN] Ruby/Odeum 0.2 (More Idiomatic, Better Source Layout) Zed A. Shaw Ruby 2 04-23-2005 05:40 PM
Is there a more idiomatic way to do this? Alan Mead Perl Misc 4 02-12-2005 04:42 AM
Idiomatic way of repeating items in a sequence. alr Python 12 07-02-2003 08:28 AM



Advertisments