Perl script for generating iTunes xml Metadata for MP3 CDs/DVDs

Question

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 <[email protected]>
# 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");

Answer 1

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).

Answer 2

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.