Hi, Thanks for the comments. 1. I will add use warnings. I don't think that it is worth the time to investigate if it runs on 5.005 or below. I'll add that as well. 2. I am not happy nor unhappy, just unaware. 3. I was unaware. I'll do that 4. Damn IDE! 5. The function is public and I didn't see the need to limit the user 6. Please explain. I don't understand 7. Why is regex injection bad? I use it for matching the files.
Best Regards, Dov Levenglick SmartDSP OS Development Leader -----Original Message----- From: Shlomi Fish [mailto:[email protected]] Sent: Wednesday, October 21, 2009 15:30 To: [email protected] Cc: Levenglick Dov-RM07994 Subject: Re: [Israel.pm] File::Find::Recursive On Wednesday 21 Oct 2009 10:34:12 Levenglick Dov-RM07994 wrote: > Hi, > I wrote a module (File::Find::Recursive) that is supposed to search for > files matching match criteria and not matching ignore criteria in folders > with match and ignore criteria. It'll call callback functions for matching > files. > > I wrote it as a complimentary to File::Find::Rule which I found to be too > ambiguous. > > Any input is welcome. I will upload to CPAN as soon as the discussion > (hopefully) winds down and the namespace is granted. > > The module can be viewed at: http://pastebin.com/m7cfc397a Well, first of all, I should note that pastebin.com does not syntax-highlight this file correctly, which makes it hard to read. Here are some notes: 1. You have "use strict;" but don't seem to have "use warnings;". Do you expect this module to work on perl-5.005 and below? 2. I see you have created File::Find::Simple -a simple alternative to File::Find. Why are you unhappy from: * http://www.shlomifish.org/open-source/projects/File-Find-Object/ Or possibly: * http://search.cpan.org/dist/File-Next/ (Which still has some of File-Find's philosophical limitations like being unable to be instantiated.). Naturally, one should note that F-F-O is a project I took over from its originator (Nanardon) and did most of my work on lately (and blogged about it). Lately, I've started working on File-Find-Object-Rule (a fork of File- Find-Rule to adapt it to File-Find-Object, which required quite a few changes and broke the external interface) and porting some of F-F-R's plugins to it. 3. According to PBP - all functions should have an explicit return at the end. 4. You seem to mix tabs and spaces inconsistently. 5. <<<< my ($self, $attr, @val) = @_; >>>> - @val is better passed as an array ref (though it's a topic of much heated debate). PBP seems to agree with me on it. 6. You seem to also invent another attribute module. Why can't you use Class-XSAccessor , Moose or possibly even Class-Accessor? 7. You have: <<<< next if grep /$file/, @{$self->{"_IGNORE_FILE_PATTERN"}}; >>>> Shouldn't you use a hash here instead, or at least \Q and \E ? This code smells of regex code injection (similar to SQL injection and XSS/HTML- injection). -------------------------- That's all I could find for now. Regards, Shlomi Fish > > > > Best Regards, > Dov Levenglick > SmartDSP OS Development Leader, > DevTech, Technology and System Organization > Freescale Semiconductor Israel > Tel. +972-9-952-2804 > The information contained in this email is classified as: > [ ] Freescale General Business Information > [ ] Freescale Internal Use Only > [ ] Freescale Confidential Proprietary > [x] Personal Memorandum > SAVE PAPER - THINK BEFORE YOU PRINT > > _______________________________________________ > Perl mailing list > [email protected] > http://mail.perl.org.il/mailman/listinfo/perl > -- ----------------------------------------------------------------- Shlomi Fish http://www.shlomifish.org/ What does "Zionism" mean? - http://shlom.in/def-zionism Chuck Norris read the entire English Wikipedia in 24 hours. Twice. _______________________________________________ Perl mailing list [email protected] http://mail.perl.org.il/mailman/listinfo/perl
