Stupid Mistakes

[inthepickle]

Apparently I am making a stupid mistake, but I can't see it. I have a
text file with one 6 digit number per line which corresponds to a file
name on a network directory. I want to find each file, then copy it
somewhere. I am trying to do it in steps to make the find a little
faster. If it finds the file in the first step, it should not go
through to the others, but it does. Testing a file, I found it in the
fist step. The length of $f was 38. Can't figure out why it is not
picking up on the length of $f in the second step. What am I doing
wrong? Please help!


use File::Find ;
system ( "cls" ) ;
$incoming = "D:/Cutlist.txt" ; #directory to look in for job sheets:
$destination = "D:/SWFiles/"; #copy destination
open INC, $incoming ; #open file
@cutlist = <INC> ; #assign file to array
close INC ; #close file
foreach ( @cutlist ) { #for every number in file
$swfile = $_ ;
chop ( $swfile ) ;
$subdir = substr ( $swfile,0,4 ) ; #get first 4 digits
print "Searching Y:/SolidWorks Files/$subdir\n" ;
@filetree = ( "Y:/SolidWorks Files/$subdir" ) ; #set to part sub dir
find ( \&findswfile,@filetree ) ;
if ( length ( $f ) < 2 ) { # if subdir finds failed
print "Searching Y:/SolidWorks Files/\n" ;
@filetree = ( "Y:/SolidWorks Files/" ) ; #set to sw dir
find ( \&findswfile,@filetree ) ;
} ; #end if for sw dir
if ( length ( $f ) < 2 ) {# if subdir & swdir finds failed
print "Searching Y:/\n" ;
@filetree = ( "Y:/" ) ; #set entire drive
find ( \&findswfile,@filetree ) ;
} ; #end if for entire drive
}; #foreach
print "\nEnd Program\n" ;
sub findswfile {
if ( ( /$swfile/ ) & ( /.SLDPRT/ | /.sldprt/ ) ) { #if part and
(caliptal or lower) match
$f = $File::Find::name ; #directory and file name
print " Found: $f\n\n" ;
system ( "copy \"$f\" \"$destination\" >nul" ) ; #copy file to
destination
} else {
return ; #try again
} ; #end file match
} ; #endsub

 

[A. Bax]

I have a text file with one 6 digit number per line which corresponds to a
file name on a network directory. I want to find each file, then copy it
somewhere. I am trying to do it in steps to make the find a little faster.
If it finds the file in the first step, it should not go through to the
others, but it does. Testing a file, I found it in the fist step. The length
of $f was 38. Can't figure out why it is not picking up on the length of $f
in the second step. What am I doing wrong? Please help!

You have the wrong idea of what the find() function does. It visits
EVERY file
in the directory tree that you specify and calls findswfile() for it.
It does
not stop doing so when you decide that you found your file and print
"Found".
After executing the copy, findswfile() will return to the calling
find() which
will then go on processing the next file in the tree. The fact that
you omitted
the 'return' statement from that part of the if branch does not mean
that the
subroutine will not return! It will return when the closing '}' is
reached.

If you want to stop find() processing any more files, you have to take
some
special measures, which is setting the $File::Find:rune variable to
a true
value as soon as findswfile() is called. This involves setting a
global
variable to keep track whether the target file was found or not.

our $f = "";
find(\&findswfile, $topdir);
...

sub findswfile
{
if (our $f) {
# do not look further if we found our target
$File::Findrune = 1;
return;
}

# Note some fixes here: use &&, not &; use ||, not |; use \.
for
# matching of a literal full stop; collapse /\.sldprt/ || /
\.SLDPRT/
# into /\.sldprt/i . There is still a big performance hit
because the
# regexp /$swfile/ must be compiled for every call to
findswfile(), but
# fixing that requires more work.

if (/$swfile/ && /\.SLDPRT/i) {
#if part and (caliptal or lower) match
our $f = $File::Find::name; #directory and file
name
print " Found: $f\n\n";
system("copy \"$f\" \"$destination\" >nul"); #copy file to
destination
}
}

__END__

 

[inthepickle]

FYI - It was a stupid mistake. My text file had blank lines at the
end of the list.

 

[inthepickle]

FYI - It was a stupid mistake. My text file had blank lines at the
end of the list.

 

[Ben Morrow]

Apparently I am making a stupid mistake, but I can't see it. I have a
text file with one 6 digit number per line which corresponds to a file
name on a network directory. I want to find each file, then copy it
somewhere. I am trying to do it in steps to make the find a little
faster. If it finds the file in the first step, it should not go
through to the others, but it does. Testing a file, I found it in the
fist step. The length of $f was 38. Can't figure out why it is not
picking up on the length of $f in the second step. What am I doing
wrong? Please help!

You need

use strict;
use warnings;

here, and you need to fix all the errors 'use strict' gives you.

use File::Find ;
system ( "cls" ) ;

While this is OK, I'd rather do it in Perl.

use Term::ANSIScreen qw/cls/;

cls;

$incoming = "D:/Cutlist.txt" ; #directory to look in for job sheets:

my $incoming = ...

and so on elsewhere.

$destination = "D:/SWFiles/"; #copy destination
open INC, $incoming ; #open file

*Always* check the return value of open.
Use three-arg open.
Use lexical filehandles.
</scratched record>

open my $INC, '<', $incoming
or die "can't read '$incoming': $!";

@cutlist = <INC> ; #assign file to array
close INC ; #close file

....and then you don't need to explicitly close, since you're not
checking the return value anyway (there's no need here).

my @cutlist = do {
open my $INC, '<', $incoming or die ...;

foreach ( @cutlist ) { #for every number in file
$swfile = $_ ;

You're just clobbering $_ for no reason.

foreach my $swfile ( @cutlist ) {

chop ( $swfile ) ;

Use chomp, not chop. Also, you can chomp a whole array at once, with

chomp @cutlist;

before the loop.

$subdir = substr ( $swfile,0,4 ) ; #get first 4 digits
print "Searching Y:/SolidWorks Files/$subdir\n" ;
@filetree = ( "Y:/SolidWorks Files/$subdir" ) ; #set to part sub dir

Why are you using an array when it only ever has one element? Why do you
specify the path twice?

my $filetree = "Y:/SolidWorks Files/$subdir";
print "Searching $filetree\n";

Also, I would set $\ = "\n" at the top and leave it out of all the print
statements.

find ( \&findswfile,@filetree ) ;
if ( length ( $f ) < 2 ) { # if subdir finds failed

What's $f? ...oh, I see, it's a global set by findswfile. This is a very
bad idea: I had to look quite hard to work out where that value was
coming from. You're in a slightly tricky situation here: File::Find's
API is not terribly well designed (it's a translation of the find.pl
library that used to come with perl 4, so it's kept some bad Perl-4ish
habits ), and it doesn't make returning a result very easy. At the
very least, using a more distinctive name and putting

my $Found; # set by findswfile

(using Initial_Caps for globals and ALL_CAPS for constants is a useful
habit to get into) just above the foreach loop would have saved me (and
you, when you come back to this in six months' time) some searching. You
will also need to explicitly set it to undef at the top of the loop, as
otherwise it will still be set to the last file you found. This is the
problem you actually asked about . I'll show you a better solution at
the end.

Testing for length < 2 here is not very clear: what you care about is
that $f has been set at all, so just use

if ($Found) {

print "Searching Y:/SolidWorks Files/\n" ;
@filetree = ( "Y:/SolidWorks Files/" ) ; #set to sw dir

Again you're duplicating the path. Every time you specify a literal like
this more than once, it's a bug waiting to happen when you need to
change the base path. You want something like

my $BASEDRIVE = 'Y:';
my $BASEPATH = "$BASEDRIVE/SolidWorks Files';

at the top, and then use

my $filetree = "$BASEPATH/$subdir";

above and $BASEPATH here. For more portability (and clarity) you could
use File::Spec, but that's likely not very useful in a
situation-specific program like this.

find ( \&findswfile,@filetree ) ;
} ; #end if for sw dir

You don't need that ';'.

The point of using sensible indentation, and blank lines between
sections of code, is to make comments like this unnecessary. Again, it's
just waiting for you to change something, so it can sit there being
wrong and confusing you .

if ( length ( $f ) < 2 ) {# if subdir & swdir finds failed
print "Searching Y:/\n" ;
@filetree = ( "Y:/" ) ; #set entire drive
find ( \&findswfile,@filetree ) ;
} ; #end if for entire drive
}; #foreach
print "\nEnd Program\n" ;
sub findswfile {
if ( ( /$swfile/ ) & ( /.SLDPRT/ | /.sldprt/ ) ) { #if part and
(caliptal or lower) match

Don't use bitwise operators (& and |) when you mean logical operators
(&& and ||). At some point they won't do what you expect.

'.' is a special character in a regex, and needs escaping with \. When
interpolating into a regex you should usually use \Q..\E, as otherwise
special characters in $swfile will be interpreted by the regex engine.

If you really mean .SLDPRT or .sldprt *only*, this is fine, but if names
like 00000.sldPRT are OK (or don't occur) you can make this clearer:

if ( / \Q $swfile \E \.sldprt /xi )

(I'm making lots of assumptions about what your filenames actually look
like, here, and I may be wrong. In that case, ignore me .)

$f = $File::Find::name ; #directory and file name
print " Found: $f\n\n" ;
system ( "copy \"$f\" \"$destination\" >nul" ) ; #copy file to
destination

You can use qq{} to avoid escaping the quotes

system( qq{copy "$f" "$destination" >nul} );

Also, I don't think DOS copy supports using / as a directory separator,
so you may need to convert it

(my $DOSfile = $f) =~ s,/,\\,g;

Depending on how much you care about creation dates &c., you may find
File::Copy easier. You could also call CopyFile directly using
Win32API::File.

} else {
return ; #try again
} ; #end file match
} ; #endsub

I would write this using File::Find::Rule, which makes it much easier.
Something like (untested)

use File::Find::Rule;
use Win32API::File qw/CopyFile/;

for my $swfile (@cutlist) {
my $subdir = substr $swfile, 0, 4;

for my $base ("$BASEPATH/$subdir", $BASEPATH, "$BASEDRIVE/") {

print "Searching $base";

File::Find::Rule
->name(qr/\Q $swfile \E \.sldprt/xi)
->exec(sub {
my ($src, $dest) = ($_[2], "$destination/$_");

print " Found: $src";
CopyFile $src, $dest
or die "can't copy '$_[2]' to '$dest': $^E";
})
->in($base)
and last;
}
}

Notice how the inner loop tries each of your alternatives in turn, and
the 'and last' jumps out as soon as one of them found something.

Ben

 

[Josef Moellers]

Why so complicated? You don't need another call to findswfile(): just
set $File::Findrune = 1 at the place where you decide that you don't
need to go further.

our $f = "";
find(\&findswfile, $topdir);
...

sub findswfile
{

# if (our $f) {
# # do not look further if we found our target
# $File::Findrune = 1;
# return;
# }

[...]
if (/$swfile/ && /\.SLDPRT/i) {
#if part and (caliptal or lower) match
our $f = $File::Find::name; #directory and file
name
print " Found: $f\n\n";
system("copy \"$f\" \"$destination\" >nul"); #copy file to
destination $File::Findrune = 1;
}
}

 

[inthepickle]

Thanks for all of the input. I am no programmer and I am fairly new
to Perl. I got a lot of good suggestions here, and I will try to
incorporate them.

Thanks again.

 

[Ben Morrow]

[please don't top-post]

Why so complicated? You don't need another call to findswfile(): just
set $File::Findrune = 1 at the place where you decide that you don't
need to go further.

The OP was attempting the search in this order:

Y:/BASEDIR/SUBDIR
Y:/BASEDIR
Y:/

, the point being that Y:/BASEDIR/SUBDIR is searched *before* the rest
of Y:/BASEDIR (presumably s?he feels the file is more likely to be found
there). While this could be emulated with one find, with bydepth => 1
and preprocess set to a sub that sorted BASEDIR/SUBDIR and BASEDIR first
in their respective directories, three finds are probably just as clear,
especially given the OP's apparent inexperience with Perl. Searching
SUBDIR three times I would expect to have negligible effect, as the
directories will all be in the cache.

Ben

 

[Josef Moellers]

[please don't top-post]

I didn't top-post, I quoted your code and amended it ;-)

As for your reply: I don't quite understand how setting a global
variable and checking it immediately on the next entry to findswfile()
and aborting *then* is better than aborting immeiately the file is found?

 

[Ben Morrow]

[please don't top-post]

I didn't top-post, I quoted your code and amended it ;-)

Not my code. You were replying to 'A. Bax'.

As for your reply: I don't quite understand how setting a global
variable and checking it immediately on the next entry to findswfile()
and aborting *then* is better than aborting immeiately the file is found?

Ah. I misunderstood you. Had your reply been properly below the piece of
code you were talking about, that wouldn't have happened.

Ben

 

[Ben Morrow]

[please wrap articles at ~72 characters]

# matching of a literal full stop; collapse /\.sldprt/ || /
\.SLDPRT/
# into /\.sldprt/i .

These don't do the same thing. The latter matches '.SlDpRt'. This may or
may not be important to the OP.

There is still a big performance hit
because the
# regexp /$swfile/ must be compiled for every call to
findswfile(), but
# fixing that requires more work.

No, there isn't. Perl keeps track of which variables were interpolated
into a regex, and only recompiles when they are changed. Thus, this will
only be recompiled once for each time through the outermost loop, as
desired.

our $f = $File::Find::name; #directory and file
name

Don't use our for simple file-scope variables. Use my at the top of the
file. our is for when you really need a package variable: generally,
when code outside this file needs access to it.

Ben