Re: Stupid Mistakes


Quoth inthepickle <inthepickle@xxxxxxxxx>:
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 ...;
<$INC>;
};

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

.