i dont like the way my code looks but work

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

While below works, I just don't like the way it looks..
I hate to put 2 similar looking loop back to back(while loop)
Also, I think I could have used grep but the way I wrote below does not work.. is grep better choice and if so, what should it look like?
I am working on converting this simple file into sub routines(why? cause I want to learn to put everything into sub) & will post it soon

use strict;

my %total;

while (<DATA>) {
  next if /^#/;
  next if /^$/;
  next if ! /(server[1-4])\s+(proc[A-Z]+)\s+(proc\.[0-9]+)\s+[0-9]+\s+
+([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+:?[0-9]{4}?)\s+.+$/;
  $total{join ("\@", $3,$1)} = "$4";
}



my @matches = ();
while (<>) {
      chomp;
      my $item = $_;
      #@matches = grep {$_ eq $item}  (keys %total);
       for (keys %total) {
         push @matches, $_ if $_ eq "$item";
       }
}


for (@matches) {
    print "$_ ===> $total{$_}\n";
}
[download]

Comment on i dont like the way my code looks but work Download Code

Replies are listed 'Best First'.
Re: i dont like the way my code looks but work by moritz (Cardinal) on Jan 25, 2008 at 08:58 UTC
The contents of your two loops don't look very similar at all, so I don't think you can unify them easily. But this: `for (keys %total) { push @matches, $_ if $_ eq "$item"; }` [download] looks like it could be simplified to: `push @matches, $item if exists $total{$item};` [download]	[reply] [d/l] [select]
Re^2: i dont like the way my code looks but work by alexm (Chaplain) on Jan 27, 2008 at 20:21 UTC
Dominus talked about this recently on his blog: Clubbing someone to death with a loaded Uzi. It's worth reading.	[reply]
Re^3: i dont like the way my code looks but work by convenientstore (Pilgrim) on Jan 27, 2008 at 22:15 UTC
hey thank you for the good pointer to that website.. I really like Mark Dominus's articles... the fact that I understood where he was going w/ below code got me really happy... After about close to year of struggle w/ perl(and still struggling mightly w/ it), I feel I have some understanding in some area~<very very limited> thanks again! `for my $k (keys %hash) { if ($k eq "name") { $hash{$k}++; } }` [download]	[reply] [d/l]
Re: i dont like the way my code looks but work by roboticus (Chancellor) on Jan 25, 2008 at 11:53 UTC
convenientstore: In addition to the other suggestions you've already received, you can eliminate the final loop. Currently, you're making a list of matches, then printing the list at the end of your program. If you print each match as you find it, you'll no longer need the @matches array or the final loop. ...roboticus	[reply]
Re^2: i dont like the way my code looks but work by convenientstore (Pilgrim) on Jan 25, 2008 at 15:18 UTC
thank you guys! I have modified my script I am trying to make my script look better and also trying to get use to put things in subroutines(because I will need to start building larger scripts thank you! `use strict; use diagnostics; sub init { my %total; while (<DATA>) { #my %total; #why doesn't this work?????? next if /^#/; next if /^$/; next if ! /(server[1-4])\s+(proc[A-Z]+)\s+(proc\.[0-9]+)\s+[0- +9]+\s+([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+:?[0-9]{4}?)\s+.+$/; $total{join ("\@", $3,$1)} = "$4"; } return \%total; } sub do_something { my $do_it = shift; while (<>) { chomp; my $item = $_; for (keys %{$do_it}) { print "$_ ===> $$do_it{$_}\n" if $_ eq "$item"; } } } do_something(init());` [download]	[reply] [d/l]
Re^3: i dont like the way my code looks but work by moritz (Cardinal) on Jan 27, 2008 at 19:03 UTC
If you declare `my %total` inside the loop, it will be only be visible inside the loop - and reset every time the loop starts over. Certainly not what you want.	[reply] [d/l]
Re: i dont like the way my code looks but work by poolpi (Hermit) on Jan 25, 2008 at 09:12 UTC
Maybe you can write the re with something like that: see Regexp::Common::net `/(server[1-4]) # $1 \s+ (proc [A-Z]+) # $2 \s+ (proc [.] \d+) # $3 \s+ \d+ \s+ ( $RE{net}{IPv4} ) # $4 \s+ .+ \z/xms;` [download] hth, PooLpi	[reply] [d/l]
Re: i dont like the way my code looks but work by dragonchild (Archbishop) on Jan 25, 2008 at 14:47 UTC
Your bug with grep is that you're assigning to @matches every time. Instead, the following will work: `push @matches, grep { $_ eq $item } keys %total;` [download] Of course, the exists test is better yet, but that at least helps you out of your grep problem. My criteria for good software: Does it work? Can someone else come in, make a change, and be reasonably certain no bugs were introduced?	[reply] [d/l]