The biggest bug I see is that you are calling
readdir multiple times in list context, but it will only return the directory once. If you want to iterate through it multiple times you should call it once and store that in an array. The second biggest potential cause of problems is that you are returning a reference to an array rather than the array directly, which could be a problem depending on how you are calling it.
However that said, virtually every line has "room for improvement". Here is a list of specifics:
- Get rid of the () in your function declaration. Prototypes in Perl almost certainly don't do what you think they do, and are likely to cause confusion for maintenance programmers. Consider them and idea that didn't work out and drop them.
- These days instead of saying DIR you can say my $dir and have a private variable. Private variables are good.
- You are passing in the directory to read as a global variable named $logSite. Decades of experience backed up by much research says that using global variables like this is poor practice. Perl has a perfectly good way of passing arguments to functions - use it.
- While you are at it add the directory and the error you got to the error message. You know, like perlstyle tells us to. This makes debugging problems much easier.
- Your use of foreach $pattern (@ARGV) { strongly suggests that you are not using strict.pm. Using that pragma will catch a lot of your bugs immediately, so it is well worth the little bit of declaration effort to make it work properly.
- Again using @uploads without declaring it private means that you are using a global variable with all of the resulting ability to accidentally overwrite global data. It also means that if you call your function twice, you will get bad data.
- If a file matches multiple patterns it will be in your array multiple times. Is that really what you want?
- There is no need to close the directory if you are not checking for errors. (Theoretically checking for errors there is a good idea. I am willing to be sloppy there though because the odds of an error are so low.)
- I would return @uploads rather than \@uploads to reduce potential surprise depending on how it is called.
Here is a rewritten version of your function. I have left your naming convention and indentation style alone because they are reasonable if different from my usual:
sub getfiles
{
my ($dirName, @patterns) = @_;
opendir(my $dir, $dirName) or die "Can't open '$dirName': $!";
my @return;
for my $file (readdir($dir))
{
for my $pattern (@ARGV)
{
if ($file =~ /$pattern/)
{
push @return, $file;
# This avoids having the file in the result twice
last;
}
}
}
return @return;
}
And then you call it like this:
my @uploads = getfiles($logSite, @ARGV);