PERL Beginners - critique me script!

Author

critique me script!

Christopher Spears

2004-12-21, 3:55 am

Here it is! Thanks for the help! Any critiques would
be appreciated!

#!/usr/bin/perl -w
use strict;

my $Pixarport = 7498;
my $host = "lex";
my $Pixarfile = $Pixarport."@".$host;
my $handles;
my $features;
my $count = 0;
my $answer;

print "Running check_licenses.\n";
my @input = `check_licenses`;
my $file_length = scalar(@input);
chomp(my $userid = `whoami`);

while (1) {
print "Remove licenses for user $userid (Y or
N)?";
chomp($answer = <STDIN> );
if ($answer =~ /^[nN]$/) {
print "Enter your userid: ";
chomp($userid = <STDIN> );
last;
}elsif ($answer =~ /^[yY]$/) {
print "$userid is your userid.\n";
last;
}else{
print "Character(s) unrecognizable.\n";
}
}

print "Remove all licenses at once (Y or N)?\n";
chomp($answer = <STDIN> );

foreach (my $line_number = 0; $line_number <=
$file_length; $line_number++){
last if ($line_number == $file_length);
my $line = $input[$line_number];
next unless $line =~ /\S/;
$line =~ s/^\s+//;
my @features_array = split /\s/, $line ; #split
$line along whitespace and place into an array
LOOKING_USERS: if ($features_array[0] eq "Users")
{
($features = $features_array[2]) =~ s/://;
#assign second element into $features and remove colon
$line_number++;
}
$line = $input[$line_number];
if ($line =~ /Users/) {
@features_array = split /\s/, $line;
#split $line along whitespace and place into an array
goto LOOKING_USERS;
}
next unless $line =~ /\S/;
$line =~ s/^\s+//;
my @handles_array = split /\s/, $line; #split
$line along whitespace and place into an array
my $sizeof = scalar(@handles_array);
if ($sizeof >= 9 && $handles_array[0] eq $userid){
($handles = $handles_array[5]) =~ s/\),//;
#assign ninth element into $handles and remove ),
my $string = join " ", @handles_array;
while (1) { #determines if licenses are
removed all at once or one at a time
if ($answer =~ /^[nN]$/) {
print "$features $string\n";
print "Remove (Y or N)?\n";
print "lmutil lmremove -c $Pixarfile
-h $features $host $Pixarport $handles\n";
chomp($answer = <STDIN> );
REMOVE_FEATURES: if ($answer =~
/^[yY]$/) {
#print "$string removed\n";
#print "lmutil lmremove -c
$Pixarfile -h $features $host $Pixarport $handles\n";
#system "lmutil", "lmremove", "-c",
$Pixarfile, "-h", $features, $host, $Pixarport,
$handles;
print "$features and $string
removed.\n";
$answer = "n";
$count++;
last;
} elsif ($answer =~ /^[nN]$/) {
print "Skipped $features and
$string.\n";
$count++;
last;
} else {
print "Character(s)
unrecognizable.\n";
print "Remove $features (Y or
N)?\n";
chomp($answer = <STDIN> );
goto REMOVE_FEATURES;
}
}elsif ($answer =~ /^[yY]$/) {
print "$features and $string
removed\n";
print "lmutil lmremove -c $Pixarfile
-h $features $host $Pixarport $handles\n";
#system "lmutil", "lmremove", "-c",
$Pixarfile, "-h", $features, $host, $Pixarport,
$handles;
$count++;
last;
}else {
print "Character(s)
unrecognizable.\n";
print "Remove all licenses at once (Y
or N)?\n";
chomp($answer = <STDIN> );
}
}
}
}

if ($count == 0) {
print "No licenses found.\n";
}

=====
"I'm the last person to pretend that I'm a radio. I'd rather go out and be a color television set."
-David Bowie

"Who dares wins"
-British military motto

"The freak is the norm." - "The Infernal Desire Machines of Dr. Hoffman" by Angela Carter

JupiterHost.Net

2004-12-21, 3:55 am

Christopher Spears wrote:
> Here it is! Thanks for the help! Any critiques would
> be appreciated!

I have a couple :)

> #!/usr/bin/perl -w
> use strict;
>
> my $Pixarport = 7498;

Is this script for pixar animation or ???

> my $host = "lex";
> my $Pixarfile = $Pixarport."@".$host;
> my $handles;
> my $features;
> my $count = 0;
> my $answer;
>
> print "Running check_licenses.\n";
> my @input = `check_licenses`;
> my $file_length = scalar(@input);
> chomp(my $userid = `whoami`);
>
> while (1) {
> print "Remove licenses for user $userid (Y or
> N)?";
> chomp($answer = <STDIN> );
> if ($answer =~ /^[nN]$/) {
> print "Enter your userid: ";
> chomp($userid = <STDIN> );
> last;
> }elsif ($answer =~ /^[yY]$/) {
> print "$userid is your userid.\n";
> last;
> }else{
> print "Character(s) unrecognizable.\n";
> }
> }

Be careful with hard coded infinite loops...
Why not kill two birds with one stone:

my $answer = 'n'; # no uninitialized value warnings, one bird...
....
print "Remove licenses for user $userid (Y for yes or anything else for
No)? ";
chomp($answer = <STDIN> );
if ($answer =~ m/^[yY]$/) {
print "Enter your userid: ";
chomp($userid = <STDIN> );
} else {
print "$userid is your userid.\n";
}
# no infinite loop, and less code = birds 2 and 3 ;p

I'd recommend avoiding while(1) loops unless you have a really really
good reason to, and there aren't many if any...

> print "Remove all licenses at once (Y or N)?\n";

....

> while (1) { #determines if licenses are

again, while(1) why? there is a better way...

> chomp($answer = <STDIN> );
> goto REMOVE_FEATURES;

I seem to remember someone saying goto was slow/pointless/not portable
or something that made me think there were better ways, but I could be
wrong like living in a cave just outside the city, sure its there but
wouldn't a house be better, something like that, a bit foggy - anyone ?? :)

see perldoc -f goto

....

> if ($count == 0) {
> print "No licenses found.\n";
> }

my preference for several reasons:

print "No licenses found.\n" if !$count;

Sponsored Links