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

I was wondering if you wouldn't mind reviewing my script I ahve gotten the end result to work but there may be an easier way of doing it.
#!/usr/bin/perl -w open PASSWD, '/etc/passwd'; my $line; my @linesplit; my @arrary1; my @arrary3; while (<PASSWD>){ $line = $_; @linesplit = split(/:/,$line); @arrary1 = "$linesplit[0]"; @arrary3 = "$linesplit[2]"; my %where; @where{"@arrary1\t"} = "@arrary3\n"; @newarray = grep {$where{$_} > 1000} keys %where; foreach $i (@newarray) { system ("find '/home/u1' -user $i -print > $i"); } }

Replies are listed 'Best First'.
Re: uid file finder
by mirod (Canon) on Jun 07, 2001 at 10:15 UTC

    My rewrite is pretty similar to DBX's.

    The main points:

    • always, ALWAYS, use strict and -w,
    • you use arrays in a lot of places where you really should be using scalars,
    • you don't need a hash (%where if you are only going to ever use it's keys, you can use an array instead,
    • you had 2 nested loops, where the second one (the foreach should really be outside. Actually you don't even need that extra loop, you can just do the find if the uid is a user uid,
    #!/usr/bin/perl -w use strict; # better to have it as a clearly defined parameter # mine is 500 for example my $MIN_USER_UID = 1000; $|++; open PASSWD, '/etc/passwd'; my @user; while (<PASSWD>){ my @field = split /:/; system ("find '/home/u1' -user $field[0] -print >$field[0]") if($fi +eld[2] > $MIN_USER_UID); }
Re: uid file finder
by DBX (Pilgrim) on Jun 07, 2001 at 09:56 UTC
    Try this for starters (not meant to be a comprehensive rewrite):
    #!/usr/bin/perl -w open PASSWD, '/etc/passwd'; my @newarray; while (<PASSWD>){ my ($user,undef,$uid) = split(/:/); push (@newarray,$user) if $uid > 1000; } foreach $i (@newarray) { system ("find '/home/u1' -user $i -print > $i"); }
    Thanks to vynce for pointing out the undef. Also to jeffa for:
    my ($user,$uid) = (split(/:/))[0,2];
Re: uid file finder
by lemming (Priest) on Jun 07, 2001 at 12:12 UTC

    And for something a bit different
    This just writes out to STDOUT with username and filename as it finds the file. This may be quicker if you have a lot of users scattered since you only do one pass on the filesystem. Then you have to process the file, which may eat that savings...

    mirod & DWS have valid comments and the only thing they didn't mention that I would is to check if passwd gets opened correctly.

    Update:
    jeffa points out that checking if /etc/passwd is opened might be pointless. You've got more problems if that fails. Such as you may be on a different OS, but then why this program? I guess I'm trained to check my opens.

    use File::Find; use File::stat; use strict; use warnings; my $dir_search = "/home/u1"; #I'd rather do a cmd line arg my $min_uid = 1000; #but for this... my %hash; open PASSWD, '/etc/passwd' or die "Could not open /etc/passwd"; while (<PASSWD>){ my ($user, $toss , $uid) = split(/:/); $hash{$uid} = $user if $uid > $min_uid; } close(PASSWD); my @uids = keys %hash; File:find (\&wanted, $dir_search); exit; sub wanted { return if (-d $_); #Let's just list files my $st = stat($_); next unless defined($st); #Diagnostics may be wanted here if (grep($st->uid, @uids)) { print $hash{$st->uid}, "\t", $File::Find::name, "\n" ; } } # on slices # Vynce suggested: my ($user, undef, $uid) = split(/:/); # jeffa this: my ($user,$uid) = (split(/:/))[0,2];
Re: uid file finder
by Jouke (Curate) on Jun 07, 2001 at 13:50 UTC
    I tried to clean up your code and came up with this:
    #!/usr/bin/perl -w use strict; # Always use strict; open(PASSWD, '/etc/passwd') || die "Could not open /etc/passwd: $!\n"; my %where; while (<PASSWD>) { my @linesplit = split /:/; # No need to use $line, # @array1 or @array3 @where{$linesplit[0]} = $linesplit[3]; } close (PASSWD) || die "Error closing file: $!\n"; #Close an opened fil +e! foreach my $i (grep {$where{$_} > 1000} keys %where) { system ("find '/home/u1' -user $i -print > $i"); }
    Some notes:
    • You should always use strict. You declared some variables lexically and others not. Strict would force you into declaring every variable
    • I don't see why you use @array1 and @array3. My version does the same and does not use them
    • Why did you add the \t and the \n to the line where you build the %where hash?
    • It makes no sense to build the where hash, and flush it immediately after each iteration of the while loop. That is what you're doing. That's why I declared %where before the while loop and moved the foreach loop out of that while
    • Update: thanks to OeufMayo: always check the return values of open() and close()
    HTH


Jouke Visser, Perl 'Adept'
Using Perl to help the disabled: pVoice and pStory
Re: uid file finder
by clintp (Curate) on Jun 07, 2001 at 17:24 UTC
    All of these solutions seem to have a really serious problem: you're passing the same file tree over-and-over again, using an external program (find) to collect data that's easily accessible to perl.

    My interperter is being updated, so here's the start of a solution for 1-pass. A structure is built that just has to be dumped to disk however you want.

    Untested, of course.

    use File::Find; sub wanted { $a=$File::Find::name; return if -l $a; # Necessary? $o=(stat($a))[4]; push( @{$catalog{$o}}, $a); } find(\&wanted, "/home/ul");