sorting and resorting hash

  • Thread starter Andrea Spitaleri
  • Start date
A

Andrea Spitaleri

Hi,
the below code works great but I would like to receive some comment
about its perl-ish style. (I am not new posting this kind of messagge,
but I want improve my style) :)
the problem is: I have several file, which they need to be ranked.
i.e.:
1.sft contains rms 1
2.sft contains rms 3
3.sft contains rms 2
4.sft contains rms 0.5
5.sft contains rms 3
........
and I would like to get at the end symbolic link, ranking the file bye
rms values:
rank_1.sft ---> 4.sft
rank_2.sft ---> 1.sft
rank_3.sft ---> 3.sft
rank_4.sft ---> 2.sft
rank_5.sft ---> 5.sft
.........
#!/usr/bin/perl

use warnings;
foreach my $index (1 .. $ARGV[0]){
open (FILE,"<$index.sft") || die "cannot open $index.sft : $!";
while($line=<FILE>) {
chomp $line;
if($line=~/^rms/){
@info=split(/ /,$line);
push @rms,$index,$info[1];
%rms = @rms;
}
}
}
print "@rms\n";
print "file ==> value rms\n";
sub tmp {
$rms{$a}<=>$rms{$b};
}
foreach $key (sort tmp (keys (%rms))){
# print "$rms[$key] --> $key.sft -->> $rms{$key}\n";
print "$key.sft -->> $rms{$key}\n";
push @new,$key;
}
for ($i=1;$i<=$#new+1;++$i){
print "$i\_rank -->> $new[$i-1].sft\n";
system ("ln -s $new[$i-1].sft $i\_rank.sft");

}

# thanks
# regards
# xspirix
 
P

Paul Lalli

Hi,
the below code works great but I would like to receive some comment
about its perl-ish style. (I am not new posting this kind of messagge,
but I want improve my style) :)
the problem is: I have several file, which they need to be ranked.
i.e.:
1.sft contains rms 1
2.sft contains rms 3
3.sft contains rms 2
4.sft contains rms 0.5
5.sft contains rms 3
.......
and I would like to get at the end symbolic link, ranking the file bye
rms values:
rank_1.sft ---> 4.sft
rank_2.sft ---> 1.sft
rank_3.sft ---> 3.sft
rank_4.sft ---> 2.sft
rank_5.sft ---> 5.sft
........
#!/usr/bin/perl

use warnings;
foreach my $index (1 .. $ARGV[0]){
open (FILE,"<$index.sft") || die "cannot open $index.sft : $!";
while($line=<FILE>) {
chomp $line;
if($line=~/^rms/){
@info=split(/ /,$line);
push @rms,$index,$info[1];
%rms = @rms;
}
Click to expand...

First, you need to clarify something a little. Does each file contain
at most one line that matches "rms"? That seems to be what you're
implying with your code, so it's the assumption I'll make too. I would
replace your whole if statment with this:

if ($line =~ /^rms\s+(.*)/){
$rms{$index} = $1;
}

There's no reason to be adding values to an array if you're only using
that array to build a hash. Just build the hash directly. Plus this will
avoid needlessly creating the temporary @info as well as the reassigning
of the entire %rms hash each time through the loop.

}
}
print "@rms\n";
print "file ==> value rms\n";
sub tmp {
$rms{$a}<=>$rms{$b};
}
foreach $key (sort tmp (keys (%rms))){
# print "$rms[$key] --> $key.sft -->> $rms{$key}\n";
print "$key.sft -->> $rms{$key}\n";
push @new,$key;
}
for ($i=1;$i<=$#new+1;++$i){
print "$i\_rank -->> $new[$i-1].sft\n";
system ("ln -s $new[$i-1].sft $i\_rank.sft");

}
Click to expand...

Why are you creating a whole new array to just store the keys of the hash
that already exists, if you're just using that array to loop over the
keys?

[untested]
@sorted = sort tmp keys %rms;
foreach $key (@sorted) {
print "$key.sft --> $rms{$key}\n";
}

my $i = 1;
foreach $key (@sorted){
print "${i}_rank --> $key.sft\n";
system ("ln -s $key.sft ${i}_rank.sft");
$i++;
}

The "${i}" method is the standard way of setting apart the variable name
from anything else in the string, rather than backslashing the following
character.

On another note, the expression $i<=$#new+1 is ugly. Much preferred (if
you want to insist on a C-style loop, rather than a foreach loop - see
another recent thread in this newsgroup) would be $i <= @new

Paul Lalli
 
B

Ben Morrow

Quoth (e-mail address removed) (Andrea Spitaleri):
Hi,
the below code works great but I would like to receive some comment
about its perl-ish style. (I am not new posting this kind of messagge,
but I want improve my style) :)
the problem is: I have several file, which they need to be ranked.
i.e.:
1.sft contains rms 1
2.sft contains rms 3
3.sft contains rms 2
4.sft contains rms 0.5
5.sft contains rms 3
.......
and I would like to get at the end symbolic link, ranking the file bye
rms values:
rank_1.sft ---> 4.sft
rank_2.sft ---> 1.sft
rank_3.sft ---> 3.sft
rank_4.sft ---> 2.sft
rank_5.sft ---> 5.sft
........
#!/usr/bin/perl

use warnings;
Click to expand...

use strict;

my (@rms, %rms);
foreach my $index (1 .. $ARGV[0]){
Click to expand...

It is perhaps not terribly 'Perlish' to use 'foreach' as 'for' will do.
open (FILE,"<$index.sft") || die "cannot open $index.sft : $!";
Click to expand...

open my $FILE, '<', "$index.sft" or die "...";

This FH will then close at the end of the scope (as written, your code
didn't close the last FH till the end of the script).
while($line=<FILE>) {
Click to expand...

while ( said:
chomp $line;
if($line=~/^rms/){
Click to expand...

chomp;

But there's no need to as you split the line anyway.

if (/^rms/) {

etc.
@info=split(/ /,$line);
Click to expand...
^^ my
push @rms,$index,$info[1];
%rms = @rms;
Click to expand...

Why are you doing this? Why not just use %rms? Anyway, why do you make
the assignment every time through the loop? Try:

$rms{$index} = (split)[1];

or, since your keys are numeric and contiguous:

push @rms, (split)[1];
}
}
}
print "@rms\n";
print "file ==> value rms\n";
Click to expand...

$\ = "\n";
print @rms;
print "file ==> value rms";

Or what I'd actually do is add '-l' to the #! line.
sub tmp {
$rms{$a}<=>$rms{$b};
}
foreach $key (sort tmp (keys (%rms))){
Click to expand...

There's no need for a named sub:

for my $key (sort { $rms{$a} said:
# print "$rms[$key] --> $key.sft -->> $rms{$key}\n";
print "$key.sft -->> $rms{$key}\n";
push @new,$key;
}
for ($i=1;$i<=$#new+1;++$i){
Click to expand...

This is never necessary.

for my $i (0..$#new) {

(obviously you now have $i values one less than before...)
print "$i\_rank -->> $new[$i-1].sft\n";
system ("ln -s $new[$i-1].sft $i\_rank.sft");
Click to expand...

There's no need to use system: perl has a symlink function.

I think you have your data structure backwards. I would use a hash the
other way round:

#!/usr/bin/perl -l

use warnings;
use strict;

my %rms;
$/ = undef;

for my $sft (1..$ARGV[0]) {
$sft .= '.sft';
open my $FILE, '<', $sft or die "can't open $sft: $!";
<$FILE> =~ /^rms (\S+)/m and $rms{$1} = $sft;
}

my $i;
for (sort keys %rms) {
my $sym = ++$i . '_rank.sft';
print "create symlink $sym -> $rms{$_} (rms: $_)";
symlink $rms{$_}, $sym or die "symlink failed: $!";
}

__END__

Ben
 
B

Ben Morrow

Quoth Paul Lalli said:
On another note, the expression $i<=$#new+1 is ugly. Much preferred (if
you want to insist on a C-style loop, rather than a foreach loop - see
another recent thread in this newsgroup) would be $i <= @new
Click to expand...

I have to disagree here :). $#new is an ordinal, @new is a cardinal:
comparing an index to @new isn't right. A *much* better solution is, if
you're going to iterate over ordinals, iterate over ordinals:

for my $i (0..$#new) {

(or, for the pedantic,

for my $i ($[ .. $#new) {

:)

Ben
 
P

Paul Lalli

Quoth Paul Lalli said:
On another note, the expression $i<=$#new+1 is ugly. Much preferred (if
you want to insist on a C-style loop, rather than a foreach loop - see
another recent thread in this newsgroup) would be $i <= @new
Click to expand...

I have to disagree here :). $#new is an ordinal, @new is a cardinal:
comparing an index to @new isn't right. A *much* better solution is, if
you're going to iterate over ordinals, iterate over ordinals:

for my $i (0..$#new) {

(or, for the pedantic,

for my $i ($[ .. $#new) {
Click to expand...

TMTOWTDI, of course. I just can't see typing $#new+1 when @new does just
as well, and is (at least to me) just as readable, if not more.

Now, on the other hand, had the OP actually had a foreach-style loop,
starting at 0, as you suggested, I surely would have recommended $#new as
opposed to @new-1.

Paul Lalli
 
B

Brian McCauley

the below code works great but I would like to receive some comment
about its perl-ish style. (I am not new posting this kind of messagge,
but I want improve my style) :)

#!/usr/bin/perl
Click to expand...

use strict;

Putting this in your code will help you to find all the places you've
forgotten to put my.
use warnings;
foreach my $index (1 .. $ARGV[0]){
open (FILE,"<$index.sft") || die "cannot open $index.sft : $!";
while($line=<FILE>) {
Click to expand...
^^
Missing my.
chomp $line;
if($line=~/^rms/){
@info=split(/ /,$line);
Click to expand...
^^
Missing my.

Why use an array if you only want one value?
push @rms,$index,$info[1];
%rms = @rms;
Click to expand...

Why does @rms exist?

Why not just say:

$rms{$index} = $info[1];
}
}
}
print "@rms\n";
print "file ==> value rms\n";
sub tmp {
$rms{$a}<=>$rms{$b};
}
Click to expand...

tmp is a bad name for a subroutine.

Actually there's no need to make this subroutine at all.
foreach $key (sort tmp (keys (%rms))){
Click to expand...
^^
Missing my.
# print "$rms[$key] --> $key.sft -->> $rms{$key}\n";
print "$key.sft -->> $rms{$key}\n";
push @new,$key;
}
Click to expand...

Why not just:

my @new = sort { $rms{$a}<=>$rms{$b} } keys (%rms);

but as I said abouve you shouldn't use a hash at all.
for ($i=1;$i<=$#new+1;++$i){
Click to expand...
^^

Missing my.

More conventionally written.

for my $i ( 1 .. @new ) {
print "$i\_rank -->> $new[$i-1].sft\n";
system ("ln -s $new[$i-1].sft $i\_rank.sft");
}
Click to expand...

Use the builtin symlink() function rather than running an external
process.

All in all I think it can be a lot simpler:

#!/usr/bin/perl
use strict;
use warnings;

my %rms;

foreach my $index (1 .. $ARGV[0]){
open (FILE,"<$index.sft") or die "cannot open $index.sft : $!";
while(my $line = <FILE>) {
$rms{$index} = $1 if $line =~ /^rms (.*)/;
}
}

my $rank;
for my $index ( sort { $rms{$a} <=> $rms{$b} } keys %rms ){
$rank++;
print "$rank\_rank -->> $index.sft\n";
symlink("$index.sft","$rank\_rank.sft") or warn $!;
}


--
\\ ( )
. _\\__[oo
.__/ \\ /\@
. l___\\
# ll l\\
###LL LL\\
 
J

John W. Krahn

Andrea said:
the below code works great but I would like to receive some comment
about its perl-ish style. (I am not new posting this kind of messagge,
but I want improve my style) :)
the problem is: I have several file, which they need to be ranked.
i.e.:
1.sft contains rms 1
2.sft contains rms 3
3.sft contains rms 2
4.sft contains rms 0.5
5.sft contains rms 3
.......
and I would like to get at the end symbolic link, ranking the file bye
rms values:
rank_1.sft ---> 4.sft
rank_2.sft ---> 1.sft
rank_3.sft ---> 3.sft
rank_4.sft ---> 2.sft
rank_5.sft ---> 5.sft
........
Click to expand...

So, you want a more perl-ish style. Just pass the actual file names on
the command line like "yourprog.pl [1-5].sft" or "yourprog.pl ?.sft"

#!/usr/bin/perl
use warnings;
use strict;

my @rms;
while ( <> ) {
if ( /rms\s+([\d.]+)/ ) {
push @rms, [ $1, $ARGV ];
close ARGV;
}
}

my $index;
for my $file ( sort { $a->[ 0 ] <=> $b->[ 0 ] } @rms ) {
my $link = 'rank_' . ++$index . '.sft';
symlink $file->[ 1 ], $link;
print "$link -->> $file->[1]\n";
}

__END__


John
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

Sorting hash of hashes 3
READFILE sorting coding problem 3
hash of arrays 1
Push regex search result into hash with multiple values 14
sorting file according to a unicode column 17
Sorting hash of hashes 9
Hash 2
data to hash 1

Members online

No members online now.
Total: 34 (members: 1, guests: 33)
Robots: 302

Forum statistics

Threads
473,755
Messages
2,569,536
Members
45,007
Latest member
obedient dusk

Latest Threads

Top