David Vd Geer Inhuur Tbv Iplib wrote at Tue, 11 Jun 2002 15:17:57 +0200:
> Hi,
>
> I am currently almost done with my current job.
> As I am reviewing my scripts the foreach-loop started to anoy me.
It should not be the only thing, but let's talk about it later.
>
> I am afraid this is slowing down the script. Does anyone know a faster way to do the
>following :
As I always say, speed is not so important as readability :-)
>
> # ------
> open(FH, "< $groupfile");
> @usrs = <FH>;
That suggest that you read only on line of the groupfile.
Of course, @usrs forces list context -
it's hard to read late in the night.
Make it unarbitrary:
my @usrs = (<FH>);
I think, too, it's the right place to chomp here.
chomp( my @usrs = (<FH>) );
> close FH;
>
> $htusr = (grep {/htuser: /} @usrs)[0] ;
> $phi = (grep {/phil: /} @usrs)[0] ;
> $al = (grep {/all: /} @usrs)[0] ;
^^^
Shouldn't it be all ??
And philosophs are great guys,
I think full names for them are O.K. :-))
Here's an excellent possibility to talk of the excellent module
List::Util of Graham Barr.
use List::Util qw(first);
my $htusr = first {/htuser: /} @usrs;
my $phi = first {/phil: /} @usrs;
my $all = first {/all: /} @usrs;
or quite shorter:
my ($htuser, $phi, $all) = map { first {/$_: /} @usrs} qw(htuser phil all) };
>
> @htuser = split(/ /, $htusr);
> @phil = split(/ /, $phi);
> @all = split(/ /, $al);
Allthough, Perl offers to give variables the same name,
it's in 90% of the cases good to avoid it.
My personal favorite would be to split when you get the first correct group.
Of course, you can only create references by that way,
but it seems to be still nice:
my ($htuser, $phi, $all) = map { [ split / /, first {/$_: /} @usrs ] }
qw(htuser phil all) };
or perhaps:
my @group_str = my ($htuser, $phi, $all)
= map { first {/$_: /} @usrs} qw(htuser phil all) };
my ($htuser_list, $phi_list, $all_list) = map { [split / /, $_] } @group_str;
>
> foreach $htuser(@htuser) {
> chomp $htuser;
> if ( "$pwuser" eq "$htuser" ) { $group = "iclab"; }
> }
> foreach $phil(@phil) {
> chomp $phil;
> if ( "$pwuser" eq "$phil" ) { $group = "phil"; }
> }
> foreach $all(@all) {
> chomp $all;
> if ( "$pwuser" eq "$all" ) { $group = "all"; }
> }
> if(!($group)) { $group = "none"; }
If I understand these lines correctly,
you want to set the group to
all if the password user contains to the group all,
phil if the password user contains to the group phil
htuser if the password user contains to the group htuser
none otherwise.
(What I don't understand is why you chomp in the loops,
@htuser e.g. is created with a split of / / where no end of lines should be included
Only the last element of a line could still have a line end in,
that should be removed when reading the file - I'll ignore it :-))
Let's try to express the algorithm in Perl:
$group =
grep( { $_ eq $pwuser } @all ) && "iclab"
or grep( { $_ eq $pwuser } @phil ) && "phil"
or grep( { $_ eq $pwuser } @htuser ) && "htuser"
or "none";
That's still some ugly, let's try to improve.
Let's take a solution with a hash:
my %group_of_user;
$group_of_user{$_} = "iclab" foreach @htuser;
$group_of_user{$_} = "phil" foreach @phil;
$group_of_user{$_} = "all" foreach @all;
$group = $group_of_user{$pwuser} || "none";
That reads good. (allthough there's still something doubled)
Note, that there's also a chance that the algorithm has become quicker.
You needn't to chomp every username,
quite only every line of the group file.
The first method is assymptotic quicker than to grep everything
(allthough that will need perhaps 1_000_000 entries).
If you call this method more than one,
and you can restore the hash,
the speed will have a very strong increase.
> It's only a pain to correct it afterwards :(
No, it's the chance to play God with your code.
The Lord could every day see that his work was good.
6 days work and then you can relax.
Best Wishes,
Janek
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
