Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine
 
PerlMonks  

comment on

( [id://3333]=superdoc: print w/replies, xml ) Need Help??

--, because that code just hurts my eyes, even more than "PERL". ( The name of the language is "Perl", the name of the executable for running scripts is "perl", but there is no "PERL".)

Problems with the code, from first to last line:

  • neither "use warnings;" nor a -w parameter in the shebang line.
  • no "use strict;"
  • Hardcoded name of directory tree to be removed in call to deldir().
  • Misleading function name "deldir". This function attempts to delete an entire directory tree, not just a directory. "deltree" would be a better name.
  • my $dirtodel=pop; instead of my $dirtodel=shift; or my ($dirtodel)=@_;, which accesses the last argument instead of the first one, without any explaination and without any need to do so.
  • Non-portable backslash as path separator. DOS and Windows have no problems with a forward slash.
  • Misleading comment after that assignment makes a naive reader think that Linux needs a special treatment (the forward slash), whereas all other operating systems work with a backslash. In fact, the forward slash works with most operating systems including DOS, Windows, and MacOS X. Only some exotic or old systems (VMS, classic MacOS) need a different character. Delegating that problem to File::Spec (a core module delivered with perl) would completely eliminate the need to modify this code for different operating systems.
  • opendir without error checks. The code does not use autodie, so errors are not automatically handled.
  • Directory handle DIR is not localised.
  • Use of a bareword handle (DIR) instead of the modern form opendir(my $handle,$name)
  • No error check for closedir. (I regularily omit that check, too. Simply because closedir does not return any useful errors. You could pass a wrong handle to it, but that's the only error condition for closedir that I know.)
  • grep filters out all directory content that starts with one or two dots, followed by any amount of arbitary characters. So, grep also filters out all hidden directory entries on Unix-like systems. The regular expression lacks the final $.
  • grep should be moved into the readdir line, so that @files does not have to be re-assigned using a temporary list. (I think modern perl versions should be able to optimize the temporary list away, but I would not bet on it.)
  • The first map modifies each entry in the @files array returned by grep by uselessly assigning to the aliased $_, and copies the modified entry into a temporary list, which finally overwrites @files. (Again, modern perl versions may be able to optimize a little bit, but again I would not bet on it.)
  • The first map should be moved into the grep { ... } readdir line, again avoiding useless re-assignments to @files.
  • The second map again overwrites the @files array, without any need to do so. @files is never used again after this map. So, in fact, this map would better be written as an ordinary foreach loop iterating over @files.
  • The ternary ?: operator fills the @files array with the return values of unlink and deldir, but no piece of the code checks the return values. Instead of ?:, better use if (...) { ... } else { ... } here.
  • unlink without error checks, and no implicit error check by use autodie.
  • unlink called for each single file in a large list of files. unlink accepts a list of files, and after filtering out directories, unlink could delete all files at once. Of course, calling unlink with more than one file makes it very hard (or impossible) to give an exact error message with the name of the files that could not be deleted.
  • The map / ?: line suggests that deldir has a return value with the same semantics as unlink. The very first call to deldir suggests that it is a void function returning nothing useful. So, what does deldir return?
  • The very last line in deldir implicitly returns whatever rmdir returns. I have the impression that this is accidentally, because there is no explicit return and the very first call to deldir is a void call. Also, the second map is effectively a void call. On the other hand, the usual error check (rmdir ... or die ...) is also missing, autodie is not used, and the return value of rmdir has the same semantics as unlink. So, either insert an explicit return and document that the caller has to check for errors, or add an error check.

You might notice that the code contains exactly one non-empty line that I did not critisize. It's the final line "}".

Problematic recursion: In the worst case (deep tree where readdir returns subdirectories before files), this keeps a list of all files in the entire directory tree in memory.

Wrong tool:

  • Most, if not all, Unix-like systems have an external rm command that can recursively delete (parameter -r) an entire directory tree without asking questions (parameter -f). It is optimized for this job, running at least as fast and as efficient as any perl code you could write. So, on Unix-like systems, system('/bin/rm','-rf',$directoryName) and die "rm -rf $directoryname failed" is often the best solution.
  • On DOS / Windows, you can get the same effect either with the old deltree utility or the (enhanced) del command with the /s /f /q parameters.
  • For cross-platform code, File::Path::rmtree() is the portable solution. File::Path is a core module, so it is available whereever you run perl.

Using no modules here is not only stupid, but dangerous. Your code is broken in every single line except for the final bracket, uses far too much resources, and does not do what it promises to do. If your friend really runs this code, he should better get someone else to write code for him until you learned to write safe and clean code.

Alexander

--
Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)

In reply to Re^3: Removing Directory and Contents with Perl by afoken
in thread Removing Directory and Contents with Perl by htmanning

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post; it's "PerlMonks-approved HTML":



  • Are you posting in the right place? Check out Where do I post X? to know for sure.
  • Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:
    <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>
  • Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
  • Want more info? How to link or How to display code and escape characters are good places to start.
Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others wandering the Monastery: (5)
As of 2024-04-16 17:26 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found