From: Jorge on
I have a Linux directory full of shell scripts (~100 scripts with
average
size of ~60 lines) with many of them calling other scripts as
depencencies.

Not being the author of the scripts and with the immediate need to
become familiar with
the overall scope of this script directory, I needed to create a cross-
reference chart
that shows what script depends on what other script.

My Perl script (below) works in terms of doing what I need, however,
IMO, it appears to run a bit slow. I timed it at ~3.5 sec. Possibly
that is it's max performance but something tells me (from other Perl
script experiences) this one just isn't up to speed.

I would appreciate any ideas or comments.

Thanks, Jorge

#!/usr/bin/perl -l

use strict;
use warnings;

&scripts_dir('/some/dir/that/holds/scripts');

sub scripts_dir {

# set some vars
my $dir = shift;
my(@paths, $paths, @scripts, $scripts, $string);
my($lines, $leaf, $header, $header2);
local($_);

# check dir can be opened for read
unless (opendir(DIR, $dir)) {
die "can't open $dir $!\n";
closedir(DIR);
return;
}

#
# read dir, skip over system files and bak files
# build array of script names
# build array of full-path script names
#
foreach (readdir(DIR)) {
next if $_ eq '.' || $_ eq '..' || $_ =~ /\.bak$/;
push(@scripts, $_);
$paths = $dir."/".$_;
push(@paths, $paths);
}
closedir(DIR);

# open ouput file, format and print headers
open OUT, ">output" or die "cannot open output for writing $! ...
\n";
$header = sprintf("%-25s%-25s%s", "SCRIPT NAME", "FOUND IN",
"USAGE");
$header2 = sprintf("%-25s%-25s%s", "===========", "========",
"=====");
print OUT $header;
print OUT $header2;

# loop through each script name
foreach $scripts(@scripts){

# loop through each script in directory
foreach my $paths(@paths){

# get last leaf of script being searched --
# if it matches itself; skip
$leaf = get_leaf($paths);
if($scripts eq $leaf) { next;}

# open each script for searching
open F, "$paths" or die "cannot open $paths for reading
$! ...\n";

while(my $lines = <F>) {

# -l switch in place
chomp($lines);

# search for matches to the commonly-used command
syntax
if($lines =~ /\$\($scripts / || $lines =~ /\`
$scripts /){

# format to line up with headers
$string = sprintf("%-25s%-25s%s", $scripts, $leaf,
$lines);

# print to file
print OUT $string;
}
}
}
}
# close I/O streams
close(F);
close(OUT);
}

#======================
# subroutine get_leaf
#======================
sub get_leaf
{
# get arg(s)
my($pathname) = @_;

# split on leafs
my @split_pathname = split( /[\\\/]/, $pathname);

# grab last leaf
my $leaf = pop( @split_pathname );

# return bare script name (leaf)
return( $leaf );
}

__END__


From: Uri Guttman on
>>>>> "J" == Jorge <awkster(a)yahoo.com> writes:

J> #!/usr/bin/perl -l

J> use strict;
J> use warnings;

good

J> &scripts_dir('/some/dir/that/holds/scripts');

don't call subs with & as it isn't needed with () and it is perl4
style. there are other subtle issues that can bite you.

J> sub scripts_dir {

J> # set some vars
J> my $dir = shift;
J> my(@paths, $paths, @scripts, $scripts, $string);
J> my($lines, $leaf, $header, $header2);

don't declare vars before you need them. in many cases you can declare
then when first used.

J> local($_);

why? actually i recommend against using $_ as much as possible (it does
have its places) so you can used named vars which are easier to read and
make the code better

J> # check dir can be opened for read
J> unless (opendir(DIR, $dir)) {

use a lexical handle

unless (opendir(my $dirh, $dir)) {

J> die "can't open $dir $!\n";
J> closedir(DIR);

why close the handle if it never opened?

J> return;

you can just exit here instead.

J> }

better yet, use File::Slurp's read_dir.


J> #
J> # read dir, skip over system files and bak files
J> # build array of script names
J> # build array of full-path script names
J> #
J> foreach (readdir(DIR)) {
J> next if $_ eq '.' || $_ eq '..' || $_ =~ /\.bak$/;

read_dir skips . and .. for you.


J> push(@scripts, $_);
J> $paths = $dir."/".$_;
J> push(@paths, $paths);
J> }
J> closedir(DIR);

that can all be done so much simpler like this (untested):

my @paths = map "$dir/$_", grep !/\.bak$/, read_dir $dir ;

J> # open ouput file, format and print headers
J> open OUT, ">output" or die "cannot open output for writing $!
J> ...

ditto for lexical handles

J> \n";
J> $header = sprintf("%-25s%-25s%s", "SCRIPT NAME", "FOUND IN",
J> "USAGE");
J> $header2 = sprintf("%-25s%-25s%s", "===========", "========",
J> "=====");

you use the same sprintf format three times. put that into a variable
above so you can change it in one place.

J> print OUT $header;
J> print OUT $header2;

no need for two print calls.

print OUT $header, $header2 ;


J> # loop through each script name
J> foreach $scripts(@scripts){

foreach my $scripts (@scripts) {

that is how you can declare vars locally. and use more horizontal
whitespace. others need to read your code so think about them when you
write it. and YOU are an other too! :)

i noticed this below but the point is true here. don't use plural names
for singular things. $scripts has a single script name

J> # loop through each script in directory
J> foreach my $paths(@paths){

you used my there. why not in the previous loop?

J> # get last leaf of script being searched --
J> # if it matches itself; skip
J> $leaf = get_leaf($paths);
J> if($scripts eq $leaf) { next;}

slightly faster as you don't need a block entry on next. also a better
style for simple flow control like this.

next if $scripts eq $leaf ;

J> # open each script for searching
J> open F, "$paths" or die "cannot open $paths for reading

don't quote scalar vars as it can lead to subtle bugs. it is not needed here.

J> $! ...\n";

J> while(my $lines = <F>) {

$lines is a single line so make that singular. using a plural name
implies an array or array ref

J> # -l switch in place

that only affects one liners using -p or -n. no effect on regular code

J> chomp($lines);

J> # search for matches to the commonly-used command
J> syntax
J> if($lines =~ /\$\($scripts / || $lines =~ /\`
J> $scripts /){

this doesn't make sense. are you checking if the current script refers
to itself??

J> # format to line up with headers
J> $string = sprintf("%-25s%-25s%s", $scripts, $leaf,
J> $lines);

J> # print to file
J> print OUT $string;

since you just print the string, you can use printf directly.

J> }
J> }
J> }
J> }
J> # close I/O streams
J> close(F);
J> close(OUT);
J> }

J> #======================
J> # subroutine get_leaf
J> #======================
J> sub get_leaf
J> {
J> # get arg(s)
J> my($pathname) = @_;

J> # split on leafs
J> my @split_pathname = split( /[\\\/]/, $pathname);

gack!!

File::Basename does this for you and simpler.

J> # grab last leaf
J> my $leaf = pop( @split_pathname );

you could just return the popped value directly. hell, you can do that
in the split line too:

return( (split( /[\\\/]/, $pathname)[-1] );

as for speedups, i can't help much since i don't have your input
data. nothing seems oddly slow looking but your core loops are deep and
can be done better. slurping in the entire file (File::Slurp) and
scanning for those lines in one regex would be noticeably faster. and
your core logic is suspect as it doesn't seem to check for calling other
scripts from a given one.

uri

--
Uri Guttman ------ uri(a)stemsystems.com -------- http://www.sysarch.com --
----- Perl Code Review , Architecture, Development, Training, Support ------
--------- Gourmet Hot Cocoa Mix ---- http://bestfriendscocoa.com ---------
From: J. Gleixner on
Jorge wrote:
> I have a Linux directory full of shell scripts (~100 scripts with
> average
> size of ~60 lines) with many of them calling other scripts as
> depencencies.
>
> Not being the author of the scripts and with the immediate need to
> become familiar with
> the overall scope of this script directory, I needed to create a cross-
> reference chart
> that shows what script depends on what other script.
>
> My Perl script (below) works in terms of doing what I need, however,
> IMO, it appears to run a bit slow. I timed it at ~3.5 sec. Possibly
> that is it's max performance but something tells me (from other Perl
> script experiences) this one just isn't up to speed.

3 seconds isn't fast enough??.. just write it, run it, and move on..
sheesh..

>
> I would appreciate any ideas or comments.
>
> Thanks, Jorge
>
> #!/usr/bin/perl -l
>
> use strict;
> use warnings;
>
> &scripts_dir('/some/dir/that/holds/scripts');
Remove the '&' there.
>
> sub scripts_dir {
>
> # set some vars
> my $dir = shift;
> my(@paths, $paths, @scripts, $scripts, $string);
> my($lines, $leaf, $header, $header2);
> local($_);

Declare your vars in the smallest possible scope.
>
> # check dir can be opened for read
> unless (opendir(DIR, $dir)) {
> die "can't open $dir $!\n";

After die is called, nothing after this point is called..
> closedir(DIR);
> return;
> }
>
> #
> # read dir, skip over system files and bak files
> # build array of script names
> # build array of full-path script names
> #
> foreach (readdir(DIR)) {
> next if $_ eq '.' || $_ eq '..' || $_ =~ /\.bak$/;
> push(@scripts, $_);
> $paths = $dir."/".$_;
> push(@paths, $paths);
> }
> closedir(DIR);
>
> # open ouput file, format and print headers
> open OUT, ">output" or die "cannot open output for writing $! ...
> \n";
use 3 argument open.

open( my $out, '>', 'output' ) || die "can't open output: $!";
> $header = sprintf("%-25s%-25s%s", "SCRIPT NAME", "FOUND IN",
> "USAGE");
> $header2 = sprintf("%-25s%-25s%s", "===========", "========",
> "=====");
> print OUT $header;
> print OUT $header2;

Just use printf.

printf OUT "%-25s%-25s%s\n", "SCRIPT NAME", "FOUND IN", "USAGE";

or printf $out...
>
> # loop through each script name
> foreach $scripts(@scripts){
for my $script( @scripts )
>
> # loop through each script in directory
> foreach my $paths(@paths){
$path seems better than $paths
>
> # get last leaf of script being searched --
> # if it matches itself; skip
> $leaf = get_leaf($paths);
my $leaf = ( split( /\//, $paths ) )[-1];

> if($scripts eq $leaf) { next;}
next if $scripts eq $leaf;
>
> # open each script for searching
> open F, "$paths" or die "cannot open $paths for reading
> $! ...\n";
open( my $file, '<', $path ) || die "can't open $path: $!;
>
> while(my $lines = <F>) {
my $line instead of my $lines.
>
> # -l switch in place
> chomp($lines);
>
> # search for matches to the commonly-used command
> syntax
> if($lines =~ /\$\($scripts / || $lines =~ /\`
> $scripts /){
>
> # format to line up with headers
> $string = sprintf("%-25s%-25s%s", $scripts, $leaf,
> $lines);
>
> # print to file
> print OUT $string;
again printf
> }
> }

Should close F or $file here.

> }
> }
> # close I/O streams
> close(F);
^^^ should be just after the end of the while.. above.
> close(OUT);
> }
>
> #======================
> # subroutine get_leaf
> #======================
> sub get_leaf
> {
> # get arg(s)
> my($pathname) = @_;
>
> # split on leafs
> my @split_pathname = split( /[\\\/]/, $pathname);
>
> # grab last leaf
> my $leaf = pop( @split_pathname );
>
> # return bare script name (leaf)
> return( $leaf );
> }
>
> __END__
>
>
From: Jorge on
I wasn't expecting a full critique but I'll take it -- learning is
good so thanks for your effort and time.

Most of your points are obvious (once read) and some I will have to
digest.

Thanks again



On May 20, 12:47 pm, "Uri Guttman" <u...(a)StemSystems.com> wrote:
> >>>>> "J" == Jorge  <awks...(a)yahoo.com> writes:
>
>   J> #!/usr/bin/perl -l
>
>   J> use strict;
>   J> use warnings;
>
> good
>
>   J> &scripts_dir('/some/dir/that/holds/scripts');
>
> don't call subs with & as it isn't needed with () and it is perl4
> style. there are other subtle issues that can bite you.
>
>   J> sub scripts_dir {
>
>   J>     # set some vars
>   J>     my $dir = shift;
>   J>     my(@paths, $paths, @scripts, $scripts, $string);
>   J>     my($lines, $leaf, $header, $header2);
>
> don't declare vars before you need them. in many cases you can declare
> then when first used.
>
>   J>     local($_);
>
> why? actually i recommend against using $_ as much as possible (it does
> have its places) so you can used named vars which are easier to read and
> make the code better
>
>   J>     # check dir can be opened for read
>   J>     unless (opendir(DIR, $dir)) {
>
> use a lexical handle
>
> unless (opendir(my $dirh, $dir)) {
>
>   J>         die "can't open $dir $!\n";
>   J>         closedir(DIR);
>
> why close the handle if it never opened?
>
>   J>         return;
>
> you can just exit here instead.
>
>   J>     }
>
> better yet, use File::Slurp's read_dir.
>
>   J>     #
>   J>     # read dir, skip over system files and bak files
>   J>     # build array of script names
>   J>     # build array of full-path script names
>   J>     #
>   J>     foreach (readdir(DIR)) {
>   J>         next if $_ eq '.' || $_ eq '..' || $_ =~ /\.bak$/;
>
> read_dir skips . and .. for you.
>
>   J>         push(@scripts, $_);
>   J>         $paths = $dir."/".$_;
>   J>         push(@paths, $paths);
>   J>     }
>   J>     closedir(DIR);
>
> that can all be done so much simpler like this (untested):
>
> my @paths = map "$dir/$_", grep !/\.bak$/, read_dir $dir ;
>
>   J>     # open ouput file, format and print headers
>   J>     open OUT, ">output" or die "cannot open output for writing $!
>   J>     ...
>
> ditto for lexical handles
>
>   J> \n";
>   J>     $header = sprintf("%-25s%-25s%s", "SCRIPT NAME", "FOUND IN",
>   J> "USAGE");
>   J>     $header2 = sprintf("%-25s%-25s%s", "===========", "========",
>   J> "=====");
>
> you use the same sprintf format three times. put that into a variable
> above so you can change it in one place.
>
>   J>     print OUT $header;
>   J>     print OUT $header2;
>
> no need for two print calls.
>
> print OUT $header, $header2 ;
>
>   J>     # loop through each script name
>   J>     foreach $scripts(@scripts){
>
>         foreach my $scripts (@scripts) {
>
> that is how you can declare vars locally. and use more horizontal
> whitespace. others need to read your code so think about them when you
> write it. and YOU are an other too! :)
>
> i noticed this below but the point is true here. don't use plural names
> for singular things. $scripts has a single script name
>
>   J>         # loop through each script in directory
>   J>         foreach my $paths(@paths){
>
> you used my there. why not in the previous loop?
>
>   J>             # get last leaf of script being searched --
>   J>             # if it matches itself; skip
>   J>             $leaf = get_leaf($paths);
>   J>             if($scripts eq $leaf) { next;}
>
> slightly faster as you don't need a block entry on next. also a better
> style for simple flow control like this.
>
>         next if $scripts eq $leaf ;
>
>   J>             # open each script for searching
>   J>             open F, "$paths" or die "cannot open $paths for reading
>
> don't quote scalar vars as it can lead to subtle bugs. it is not needed here.
>
>   J> $! ...\n";
>
>   J>             while(my $lines = <F>) {
>
> $lines is a single line so make that singular. using a plural name
> implies an array or array ref
>
>   J>                 # -l switch in place
>
> that only affects one liners using -p or -n. no effect on regular code
>
>   J>                 chomp($lines);
>
>   J>                 # search for matches to the commonly-used command
>   J> syntax
>   J>                 if($lines =~ /\$\($scripts / || $lines =~ /\`
>   J> $scripts /){
>
> this doesn't make sense. are you checking if the current script refers
> to itself??
>
>   J>                     # format to line up with headers
>   J>                     $string = sprintf("%-25s%-25s%s", $scripts, $leaf,
>   J> $lines);
>
>   J>                     # print to file
>   J>                     print OUT $string;
>
> since you just print the string, you can use printf directly.
>
>   J>                 }
>   J>             }
>   J>         }
>   J>     }
>   J>     # close I/O streams
>   J>     close(F);
>   J>     close(OUT);
>   J> }
>
>   J> #======================
>   J> # subroutine get_leaf
>   J> #======================
>   J> sub get_leaf
>   J> {
>   J>         # get arg(s)
>   J>         my($pathname) = @_;
>
>   J>         # split on leafs
>   J>         my @split_pathname = split( /[\\\/]/, $pathname);
>
> gack!!
>
> File::Basename does this for you and simpler.
>
>   J>         # grab last leaf
>   J>         my $leaf = pop( @split_pathname );
>
> you could just return the popped value directly. hell, you can do that
> in the split line too:
>
>         return( (split( /[\\\/]/, $pathname)[-1] );
>
> as for speedups, i can't help much since i don't have your input
> data. nothing seems oddly slow looking but your core loops are deep and
> can be done better. slurping in the entire file (File::Slurp) and
> scanning for those lines in one regex would be noticeably faster. and
> your core logic is suspect as it doesn't seem to check for calling other
> scripts from a given one.
>
> uri
>
> --
> Uri Guttman  ------  u...(a)stemsystems.com  --------  http://www.sysarch.com--
> -----  Perl Code Review , Architecture, Development, Training, Support ------
> ---------  Gourmet Hot Cocoa Mix  ----  http://bestfriendscocoa.com---------

From: Jim Gibson on
In article
<6bf31a76-1cd6-48be-8bea-a91b1c6b83b6(a)y21g2000vba.googlegroups.com>,
Jorge <awkster(a)yahoo.com> wrote:

> I have a Linux directory full of shell scripts (~100 scripts with
> average
> size of ~60 lines) with many of them calling other scripts as
> depencencies.
>
> Not being the author of the scripts and with the immediate need to
> become familiar with
> the overall scope of this script directory, I needed to create a cross-
> reference chart
> that shows what script depends on what other script.
>
> My Perl script (below) works in terms of doing what I need, however,
> IMO, it appears to run a bit slow. I timed it at ~3.5 sec. Possibly
> that is it's max performance but something tells me (from other Perl
> script experiences) this one just isn't up to speed.
>
> I would appreciate any ideas or comments.
>
> Thanks, Jorge
>

Uri and J. have given you thorough critiques of your posted program. I
can only add a few additional suggestions.

Concerning speedup, you have implemented an O(N**2) algorithm. You are
reading each of N files N-1 times, looking for a different script name
each time you read each file. You should read each file only once, and
look for all of your script names in each line. You shouldn't exclude
the script your are reading from this search, since scripts can call
themselves. I would also not look in comments, since it is very common
to put script names in comments, and you don't want those.

For how to search for many things at once, see the advice in 'perldoc
-q many' "How do I efficiently match many regular expressions at once?"

You are concatenating the directory name and file name into a path,
then taking many steps to strip the directory name from the path to
retrieve the file name. Don't bother, since you are only working in one
directory and the $dir variable only contains one value. Add $dir to
the open statement or a temp variable:

my $filename = "$dir/$scripts";
open( my $f, '<', $filename ) ...

Then you don't need the @paths array at all.

[program snipped]

--
Jim Gibson