9
\$\begingroup\$

I created a short Perl 6 script copyfnameascii.pl to copy a file hierarchy I have, applying an URL Decoding to the names of folders and removing non-ASCII characters from the names of files.

$ cd workingdirectory
$ perl6-m ../copyfnameascii.pl project copy

It is best illustrated by the following diagram:

                             $ tree
                             .
                             ├── copy
                             │   ├── folder 1
$ tree                       │   │   ├── cr.txt
.                            │   │   └── es.txt
└── project                  │   └── folder 2
    ├── folder%201           │       ├── ai.txt
    │   ├── čř.txt           │       └── zy.txt
    │   └── ěš.txt   ~*>     └── project
    └── folder%202               ├── folder%201
        ├── áí.txt               │   ├── čř.txt
        └── žý.txt               │   └── ěš.txt
                                 └── folder%202
                                     ├── áí.txt
                                     └── žý.txt

I just started using Perl. I'd like to hear your advice how to make the code generally better; more readable, idiomatic, and so on. The script does work; there is just a few things I am not satisfied with.

use Inline::Perl5;
my $p5 = Inline::Perl5.new;
$p5.use('Text::Unidecode');

sub transformdirname($dirname is copy) {
  $dirname.=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g ); # undo URL encoding
  $dirname.=subst( /\//, '_', :g ); # make it a valid UNIX filename
  return $dirname;
}

sub transformfilename($filename) {
  return $p5.call('Text::Unidecode::unidecode', $filename); # make it ASCII at all costs
}

sub MAIN($fromdir, $todir) {
  for dir($fromdir) -> $subdir {
    unless $subdir ~~ :d {
      next;
    }
    my $tosubdir = $todir ~ '/' ~ transformdirname($subdir.basename);
    say $tosubdir;
    mkdir($tosubdir);
    for dir($subdir) -> $file {
      my $tofile = $tosubdir ~ '/' ~ transformfilename($file.basename);
      if $tofile.path ~~ :e {
        die("Will not overwrite $tofile");
      }
      copy($file, $tofile);
    }
  }
}
\$\endgroup\$
  • \$\begingroup\$ I don't think that this is a good task. Scroll down in this answer on so to the pure unicode-related false assumptions – you're breaking quite some of them. \$\endgroup\$ – Patrick J. S. Dec 7 '14 at 7:59
  • \$\begingroup\$ @PatrickJ.S. Can you give me the numbers of a few of them you think I am breaking? I know that directory names are in English, therefore all ASCII, so no Unicode problems there. If it wasn't true, I guess I'd just have to treat what the line # undo URL encoding gives me as bytes and decode it with UTF-8. I still believe that regarding the file names I am doing everything correctly. \$\endgroup\$ – user7610 Dec 7 '14 at 10:03
  • \$\begingroup\$ It is not a good task. But it is a necessary one. If I didn't romanize the file names, the project tracing web service where I upload the files would do it for me, replacing all UTF-8 runes that is not ASCII with underscores and adding suffixes to eliminate duplicite filenames. Then the files would be named '__1.txt', '__2.txt' and so on. I can't have that. \$\endgroup\$ – user7610 Dec 7 '14 at 10:05
  • 1
    \$\begingroup\$ You're right, it's just 42 – 47 is worth considering. for the files e.txt, è.txt ê.txt, your file won't do anything (it would die). If you really want ascii only filenames, you could double URI encode it, while not nice, you're at least guaranteed to have distinctive filenames. Furthermore, as you mentioned web services, javascript has a built-in URI-decode function \$\endgroup\$ – Patrick J. S. Dec 7 '14 at 12:03
  • \$\begingroup\$ 47 is news4me. (1) Text::Unidecode has a good rationale for Unicode to ASCII conversion. (2) I prefer nice filenames over guaranteed distinctive filenames. That's the whole reason why I am running this script instead of letting the webservice do the job its way (underscores). I'll always be there to manually fix the file names if the script ever dies, if that happens too often, I'd extend it to append distinguishing suffixes or whatever. It never died on actual input yet. This is a non-issue. Thanks for thoroughly reviewing line 12 of the script, but this is not what the question asks for \$\endgroup\$ – user7610 Dec 7 '14 at 12:44
8
\$\begingroup\$

My overall take was "Nice!". :)

Given your exchange with Patrick J S above (not to mention my lack of familiarity with the wrinkles discussed in the relevant RFCs) I'm not going to comment at all on the technical-compliance-with-standards aspects of your decoding logic.

Simplify Inline::Perl5 code

use Inline::Perl5;
my $p5 = Inline::Perl5.new;
$p5.use('Text::Unidecode');
.
.
sub transformfilename($filename) {
  return $p5.call('Text::Unidecode::unidecode', $filename);
}

That was old style. Write this way instead:

use Text::Unidecode:from<Perl5>;

sub transformfilename($filename) {
  return Text::Unidecode::unidecode($filename);
}

Don't repeat yourself?

sub transformdirname($dirname is copy) {
  $dirname.=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g );
  $dirname.=subst( /\//, '_', :g );
  return $dirname;
}

This has 'dirname' in the sub's name plus four mentions of $dirname. Sometimes repeats are valuable, perhaps making code easier to read or change, but in this case I think the reverse applies. I'd write:

sub transformdirname($_ is copy) {
  .=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g )
  .=subst( /\//, '_', :g )
}

transformfilename has a similar issue:

sub transformfilename($filename) {
  return $p5.call('Text::Unidecode::unidecode', $filename);
}

('filename' is written three times.) In addition it's a one line routine introducing the abstract "transform" and I'm going to assume the transformation is unlikely to change, so abstracting it out to a sub is questionable. In addition there's another several 'file's in the call so instead I'd elide the sub altogether and eliminate all the repeats of 'file'. We'll see that later in the final version where I've made some additional changes.

Comments

Imo good code means good comments. You've written good comments like:

sub transformdirname($dirname is copy) {
  $dirname.=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g ); # undo URL encoding
  $dirname.=subst( /\//, '_', :g ); # make it a valid UNIX filename
  return $dirname;
}

In P6 you can effortlessly turn such ordinary end-of-line comments (that start with #) into Pod declarator comments (that start with #| or #= and appear immediately before or after a declaration):

#| undo URL encoding and make it a valid UNIX filename
sub transformdirname($_ is copy) {
  .=subst( /:i \%(<[0..9A..F]>**2) /, { chr(:16(~$0)) }, :g );
  .=subst( /\//, '_', :g );
}

Now documentation tools can automatically extract documentation of the sub because this:

say &transformdirname.WHY;

now displays:

undo URL encoding and make it a valid UNIX filename

(Why is the method that extracts such doc called WHY? It's to remind folk that by far the most important thing to explain in code comments is WHY -- why the code has been written and/or why it's written the way it is.)

I note you've used MAIN:

sub MAIN($fromdir, $todir) {

As you know, this automatically generates a nice usage message. What you may not know is that this can be combined with the Pod declarator blocks discussed above to great effect. Checkout The Cool subset of MAIN for examples.

Final version

I've made many more changes to be the way I write things. As I wrote at the start, your original code looks nice and many coding issues are subjective.

use Text::Unidecode:from<Perl5>;

sub MAIN ($fromdir, $todir) {

  for $fromdir.dir {

    next unless .d;

    say my $tosubdir = "$todir/&transformdirname(.basename)";
    mkdir $tosubdir;

    for .dir {

      # make filename ASCII at all costs:
      my $tofile = "$tosubdir/{.basename.&Text::Unidecode::unidecode}";

      if $tofile.path.e { die "Will not overwrite $tofile" }

      copy $_, $tofile;

    }
  }
}

# undo URL encoding and make it a valid UNIX filename
sub transformdirname($_ is copy) {
  .=subst( /'%' (<xdigit>**2)/, { chr(:16(~$0)) }, :g )
  .=subst( /\//, '_', :g )
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.