sheridan3003 has asked for the wisdom of the Perl Monks concerning the following question:

I have written a small piece of code to cleanup the html code I have in a directory. I was wondering how this code could be improved. 
 
#!/usr/bin/perl -w
use HTML::Clean;
my @file_list = `ls *.html`;
foreach(@file_list) {
chomp();
clean_file($_);
}

sub clean_file {
    my ($filename) = shift;
    print "$filename is being cleaned!\n";
    my $h = new HTML::Clean($filename,9);
    $h->compat();
    $h->strip();
    my $myref = $h->data();
    open(OUTPUT,">$filename");
    print OUTPUT $$myref;
    close(OUTPUT);
}

[download]
I would like to improve my coding abilities and seek the wisdom of others to guide me. Thanks.

Replies are listed 'Best First'.
Re: Improvement suggestions?
by Masem (Monsignor) on Oct 14, 2001 at 02:36 UTC
    You've got -w , but not use strict. While I think your code is strict already, it's always a good idea to have this.

    It's much easier to glob a file list, than to use a system call; the former is more portable than other methods. That is, you can reduce your pre-function code to : 

    #!/usr/bin/perl -w
use strict;
use HTML::Clean;
clean_file( $_ ) foreach glob "*.html";

    [download]
    You should always catch errros from file system calls like open, that is, you should do open (OUTPUT, ">$filename") or die "Can't open $filename: $!"; to get meaningful errors.

    IMO, it would also be better to write to a temporary file then move the file over using rename than to write over an existing file. This would minimize the chance of errors losing any original HTML files to start with.

    Everything else seems good from a programmer's standpoint.

    -----------------------------------------------------
    Dr. Michael K. Neylon - mneylon-pm@masemware.com || "You've left the lens cap of your mind on again, Pinky" - The Brain
    It's not what you know, but knowing how to find it if you don't know that's important

[reply]
[d/l]
[select]
      Thanks. That is what I was looking for. This looks much more like PERL than some of the other languages that I program in.
[reply]
Re: Improvement suggestions?
by wog (Curate) on Oct 14, 2001 at 02:45 UTC
    In addition to checking open for errors, you should check new HTML::Clean(...) for errors. It's not well documented, but HTML::Clean's new will return undef if it has trouble opening the file you pass to it. (E.g. a "permission denied".) 
    my $h = new HTML::Clean($filename,9) or die "Couldn't load $filename: 
+$!\n";

    [download]
[reply]
[d/l]
[select]
Re: Improvement suggestions?
by dragonchild (Archbishop) on Oct 15, 2001 at 17:39 UTC
    I'd do the following:
    1. For readability, make the foreach look something like: 
      foreach my $filename (@file_list) {
    chomp $filename;
    clean_file($filename);
}

      [download]
    2. Don't use the indirect form of any method, especially a constructor. Tell Perl what you want instead of depending on Perl to do the right thing. It's better to be 100% safe instead of 99% safe. Instead, do: 
          my $h = HTML::Clean->new($filename, 9);

      [download]

    ------
    We are the carpenters and bricklayers of the Information Age.

    Don't go borrowing trouble. For programmers, this means Worry only about what you need to implement.

[reply]
[d/l]
[select]

Back to Seekers of Perl Wisdom