3
\$\begingroup\$

Many years ago, I wrote a script in Perl that was meant to create a cached metadata of MP3 files (in Apple's plist/XML format) which iTunes uses when you insert a CD/DVD full of MP3 files (the same would be true for AAC files, but I limited the scope of my project, due to lack of usable libraries back then).

Since my Perl is rusty, I would love to have some feedback and constructive criticism on how to make the script better, especially regarding readability.

Since the script is trivial, but long, I don't know how much I should put it here. The whole script is in one of my github repositories, if you want to see it.

An abridged version of it is:

#!/usr/bin/perl -W
# Copyright (C) 2009 Rogério Brito <rbrito@users.sf.net>
# This program is Free Software and is distributed under the terms of
# the GNU General Public License version 2 or, at your option, any
# latter version.

use strict 'vars';
use warnings;
use utf8;

use Encode qw(encode decode);
use MP3::Tag;


# ==============================================================================
# Auxiliary functions for generation of the output
sub pr_header {
print FH <<EOF
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>Current Version</key><integer>1</integer>
    <key>Compatible Version</key><integer>1</integer>
    <key>Application</key><string>m3s v0.0</string>
    <key>Burner Info</key><string>$_[0]</string>
    <key>Disc ID</key><string>$_[1]</string>
    <key>Disc Name</key><string>$_[2]</string>
    <key>tracks</key>
    <array>
EOF
}
sub pr_footer { print FH "\t</array>\n</dict>\n</plist>\n"; }


# ==============================================================================
# Functions to generate the proper XML tags for iTunes.
sub pr_open_dict  { print FH "\t\t<dict>\n"; }
sub pr_key        { print FH "\t\t\t<key>", my_utf8_encode($_[0]), "</key>"; }
sub pr_string     { print FH "<string>", my_utf8_encode($_[0]), "</string>\n"; }
sub pr_integer    { print FH "<integer>", my_utf8_encode($_[0]), "</integer>\n"; }
sub pr_date       { print FH "<date>", my_utf8_encode($_[0]), "</date>\n"; }
sub pr_boolean    { print FH ($_[0])?"<true/>\n":"<false/>\n"; }
sub pr_close_dict { print FH "\t\t</dict>\n"; }


# ==============================================================================
# Auxiliary functions
sub gen_serial_no {
    return sprintf("%04X%04X%04X%04X", rand(0xffff), rand(0xffff),
           rand(0xffff), rand(0xffff));
}

sub escape_char { my $st = shift; $st =~ s/&/&#38;/g; return $st; }
sub my_utf8_encode { return escape_char(encode("utf8", $_[0])); }

sub recurse_dir {
    my $dh;

    opendir($dh, $_[0]) or die("Error opening dir $_[0]: $!\n");

    my $par_name = $_[1];
    my $file_no = 0;
    my $dir_no = 0;

    # grep {!/\.|\.\./}
    foreach (sort readdir($dh)) { # for each entry
    my $name = "$_[0]/$_";
    if (-d $name and $_ ne "." and $_ ne "..") {
        ++$dir_no;
        recurse_dir($name, $par_name?"$par_name:$dir_no":$dir_no, $dir_no);
    } elsif ($_ ne "." and $_ ne "..") {
        ++$file_no;

        my $filename = $_[0];
        if (m!\.mp3$!i) {
        generate_song_entry("$_[0]/$_",
                    $par_name?"$par_name:$file_no":$file_no,
                    $file_no);
        }
    }
    }
    closedir($dh) or die("Error closing dir $_[0]: $!\n");
}


# ==============================================================================
# Function to grab information from an MP3 file
sub generate_song_entry {
    # initialize mp3 object
    my $mp3 = new MP3::Tag $_[0];

    # Perform the information gathering from the file:
    my ($title, $track, $artist, $album, $comment, $year, $genre) = $mp3->autoinfo();

    my ($track1, $track2, $disc1, $disc2) = ($mp3->track1(), $mp3->track2(),
                         $mp3->disk1(), $mp3->disk2());

    my ($time, $bitrate, $frequency, $is_vbr, $size) = (int($mp3->total_secs()*1000),
                            $mp3->bitrate_kbps(),
                            $mp3->frequency_Hz(),
                            $mp3->is_vbr(),
                            $mp3->size_bytes());

    # destroy object
    $mp3->close();

    # Now, we fill in the entry for this file
    pr_open_dict();

    if (defined($title))    { pr_key("Name"); pr_string($title); }
    if (defined($artist))   { pr_key("Artist"); pr_string($artist); }
    if (defined($album))    { pr_key("Album"); pr_string($album); }
    if (defined($genre))    { pr_key("Genre"); pr_string($genre); }
    if (defined($year))     { pr_key("Year"); pr_integer($year); }
    if (defined($track1))   { pr_key("Track Number"); pr_integer($track1); }
    if (defined($track2))   { pr_key("Track Count"); pr_integer($track2); }
    if (defined($disc1))    { pr_key("Disc Number"); pr_integer($disc1); }
    if (defined($disc2))    { pr_key("Disc Count"); pr_integer($disc2); }
    if (defined($time))     { pr_key("Total Time"); pr_integer($time); }
    if (defined($bitrate))  { pr_key("Bit Rate"); pr_integer($bitrate); }
    if (defined($frequency))    { pr_key("Sample Rate"); pr_integer($frequency); }
    if (defined($is_vbr))   { pr_key("Has Variable Bit Rate"); pr_boolean($is_vbr); }
    if (defined($size))     { pr_key("Size"); pr_integer($size); };

    if ((exists($mp3->{ID3v1}) or exists($mp3->{ID3v2}))) {
    pr_key("Supports ID3 Tags"); pr_boolean("true");
    }

    # Compulsory filling
    { pr_key("Date"); pr_string(12345678); };
    { pr_key("Date Modified"); pr_date("2009-03-02T19:50:00Z"); };
    { pr_key("Numeric Path"); pr_string("$_[1]"); };
    { pr_key("File Extension"); pr_string("mp3"); };

    pr_close_dict();
}


# ==============================================================================
# main program

my $root   = defined($ARGV[0])?$ARGV[0]:".";
my $file   = "$root/ContentsDB.xml";
my $title  = defined($ARGV[1])?my_utf8_encode($ARGV[1]):"My MP3 CD-ROM";
my $burner = defined($ARGV[2])?my_utf8_encode($ARGV[2]):"DVD-ROM DRIVE";
my $serial = gen_serial_no();

open(FH, ">$file") or die("Error opening file $file: $!\n");

pr_header($burner, $serial, $title);
recurse_dir($root, "");
pr_footer();

close(FH) or die("Error closing file $file: $!\n");
\$\endgroup\$
1
\$\begingroup\$

I'd replace

foreach (sort readdir($dh)) { # for each entry
my $name = "$_[0]/$_";
if (-d $name and $_ ne "." and $_ ne "..") {
    ...
} elsif ($_ ne "." and $_ ne "..") {
    ...

with

foreach my $fn (sort readdir($dh)) { # for each entry
next if $fn =~ /^\./;
my $name = "$_[0]/$fn";
if (-d $name) {
    ...
} else {
    ...

Two reasons:

  1. You want readability. So, lose generic $_ and give it some names (I often use $fn as a temporary filenname, so that's readable to me; you may have different names).

  2. Every file/dir with name starting with . is considered hidden, so it's usually good practice to skip them all.

Similar to the point 1 above, I'd do the same with @_. You can put at the beginning of each subroutine something like

my ($name1, $name2) = @_;

and then use $name1$ and $name2 instead of $_[0] and $_[1]. This makes it easier to see in your code what are $_[$i] and also makes it easier to see what arguments a certain subroutine is expecting.

This one is probably more a matter of taste:

return sprintf("%04X%04X%04X%04X", rand(0xffff), rand(0xffff),
       rand(0xffff), rand(0xffff));

can be replace by

return sprintf("%04X"x4, map { rand(0xffff) } (0..3));

I prefer the second one, because its easier to discern how many 4-digit hex numbers are there, without counting rand-s or %04X substrings.

Btw, this rand() will never give you 0xffff. If you want to include it, you need rand(0x10000).

\$\endgroup\$
  • \$\begingroup\$ thanks for the comments. I wrote that when I had not learnt about Python and I was using a lot of implicit naming in Perl (which makes it hard to follow one's own code). Regarding the random number generation, thanks for reminding me that the numbers are less than the upper limit. That's a bug in my code. Regarding the ugly %04X multiple times, it seems that I can substitute a pair of %04X with %08X and make the code slightly more readable. With %016X I wasn't successful, though... \$\endgroup\$ – rbrito Aug 11 '13 at 22:05
  • 1
    \$\begingroup\$ This might be useful, especially if the code is to run on Win (check the last comment under the answer). \$\endgroup\$ – Vedran Šego Aug 11 '13 at 22:12
  • \$\begingroup\$ Thanks. I'm actually not worried about running under windows, but that was, at the time, my understanding of the limitations of rand(), which was why I opted for calling it many times. OTOH, I would like to keep the script with fewer external dependencies, which means that I may stick with the 4 calls to rand(). \$\endgroup\$ – rbrito Aug 11 '13 at 23:01
2
\$\begingroup\$

Agree with what Vedran wrote. I'd also add some prototypes to go with that, so that perl can give you warnings or errors when you miss a parameter; e.g., something like

sub foo($$) {

}

to specify that this sub takes two scalars as parameters.

There are some limitations to what you can do with perl's prototypes like this, but it's still useful.

\$\endgroup\$
  • \$\begingroup\$ yes, the prototype thing is good. Too bad that I don't know how to make them more descriptive (say, like C or Ruby or Python or...) BTW, nice seeing you here! :) \$\endgroup\$ – rbrito Aug 11 '13 at 21:55

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.