http://qs321.pair.com?node_id=725273

convenientstore has asked for the wisdom of the Perl Monks concerning the following question:

hi folks,
below is part of my existing program(below code NOT tested) and wanted to write this in different style(or more efficient)... I just don't like the fact that I have strings of if statement like this.. any other(better) ways to write this?
use warnings; use strict; my ($newyork,$miami); while (<DATA>) { if ( $_ =~ /^NEW YORK:\s+(\S+)/ ) { $newyork = $1; $newyork = $newyork || 'NA'; next; } if ( $_ =~ /^MIAMI:\s+(\S+)/ ) { $miami = $1; $miami = $miami || 'NA'; next; } } __END__ NEW YORK: knicks CHICAGO: bulls MIAMI: heat LA: lakers

Replies are listed 'Best First'.
Re: Please fix my writing style
by dragonchild (Archbishop) on Nov 22, 2008 at 04:53 UTC
    Whenever you have a variable named after what you plan on putting in it, that's a problem. You want the variable to describe what it holds, not -be- what it holds. You want a hash.
    my %teams = map { lc } map { split /:\s+/ } map { chomp; $_ } <DATA>;
    That's probably one of the more "Perlish" ways of writing that. Your datafile's format sucks, which is your problem. Pretty code and sucky input generally don't mix. "A" input gets to play with "A" code. "C" input plays with "C" code. All the special cases.

    My criteria for good software:
    1. Does it work?
    2. Can someone else come in, make a change, and be reasonably certain no bugs were introduced?
Re: Please fix my writing style
by jwkrahn (Abbot) on Nov 22, 2008 at 02:10 UTC
    if ( $_ =~ /^NEW YORK:\s+(\S+)/ ) { $newyork = $1; $newyork = $newyork || 'NA'; next; }

    (\S+) will always match at least one character and unless you have a team named '0' (the digit zero) then 'NA' will never be assigned.

    How about like this UNTESTED:

    #!/usr/bin/perl use warnings; use strict; my %lookup = ( 'NEW YORK' => 1, 'MIAMI' => 1, ); my %data; while ( <DATA> ) { next unless /^([^:]+):\s+(\S+)/ && $lookup{ $1 }; $data{ $1 } = $2; } __END__ NEW YORK: knicks CHICAGO: bulls MIAMI: heat LA: lakers
Re: Please fix my writing style
by hydo (Monk) on Nov 22, 2008 at 03:10 UTC
    Syntactical differences aside, that's almost exactly how I've been writing code for a long time with the exception of the my statement.
    my $newyork; my $miami; if ( exp ) { statement; } else { statement; }
    Although now with 5.10 you could do
    while ( <DATA> ) { given( $_ ) { when ( $_ =~ /^NEW YORK:..../ ) { expression; } ... default { ... } } }
    Which is pretty nifty, imo. I'm sure there's even more compact ways you can say that as well. As a matter of practice though, as long as I can come back in 6 months and figure out what I was doing in roughly 30 seconds, I personally don't care about the style. Though, for some inexplicable reason, this always bugs me and I find it thoroughly distracting.
    while ( <DATA> ) { if ( expr ) { expr } }
      Although now with 5.10 you could do
      while ( <DATA> ) { given( $_ ) { when ( $_ =~ /^NEW YORK:..../ ) { expression; } ... default { ... } } }
      Which is pretty nifty, imo

      I personally believe that the most powerful characteristic of C<when>'s is the syntactic sugar it provides with implicit matches: which most of the times, actually happen to be smart matches - albeit not in this particular case, in which a similar beast that is a regular bind would be used anyway; thus you wouldn't do $_ =~ /REGEX/ but just as you wouldn't do in Perl quite about anywhere: because as usual you either want to use a full fledged generic variable and the binding operator, or the "topicalizer," i.e. $_, i.e. the pronoun "it" - but implicitly!

      --
      If you can't understand the incipit, then please check the IPB Campaign.
Re: Please fix my writing style
by juster (Friar) on Nov 22, 2008 at 07:38 UTC

    Another perlish solution:

    my %teams = do { local $/; <DATA> =~ /^([A-Z\s]+?):\s*(.+)$/mg; };

    The do block is most helpful here to override $/ in the block and leave it unchanged elsewhere. local $/; turns on file slurping. A match in list context returns a list of all our submatches in parenthesis, which go to our hash.

      btw, can i do this?
      my %teams = do { local $/; next unless <YB> =~ /^([A-Z\s]+?):\s*(.+)$/ +mg; };
      this code seems to get stuck on lines that doesn't have the format that regex is specifying.. so I wanted to skip them..

        You may want to use dragonchild's solution because it is easier to understand.

        You will need to read up on "do" to understand it better. do is not a loop, using next will only exit the block. Let me try to avoid using do.

        "Slurping" a file means reading all its data into a string. There alot of ways to do that, search for file slurping on Super Search to understand it better.

        Once you have the file data the regexp is easier to understand:

        my $teamdata; for my $line (<DATA>) { $teamdata .= $line; } # If you assign a global match to a list, it stores # everything that matched in parenthesis to that list. # A hash can be created using a list eg: (key, val, key, val) my %teams = $teamdata =~ /^([A-Z ]+): *(.+)$/mg;

        The problem with the code I supplied is \s matches whitespace, including a newline. If you replace \s with just a space character, it will skip the bad lines.

        The important thing to learn is how the global match /<regexp>/g can return a list of results, or the number of matches depending on what you assign it to. See perlretut (ie Extracting matches), perlre, search here, etc.

      thank you guys.. i like both of your sytle.. trying them out now..