Please fix my writing style

[convenientstore]

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

[dragonchild]

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.

[jwkrahn]

    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

[hydo]

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
  }
}

[blazar]

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!

[juster]

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.

[convenientstore]

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..

[juster]

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.

[convenientstore]

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