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);
Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
Read Where should I post X? if you're not absolutely sure you're posting in the right place.
Please read these before you post! —
Posts may use any of the Perl Monks Approved HTML tags:
- a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
| |
For: |
|
Use: |
| & | | & |
| < | | < |
| > | | > |
| [ | | [ |
| ] | | ] |
Link using PerlMonks shortcuts! What shortcuts can I use for linking?
See Writeup Formatting Tips and other pages linked from there for more info.