Beefy Boxes and Bandwidth Generously Provided by pair Networks
No such thing as a small change
 
PerlMonks  

Improvement suggestions?

by sheridan3003 (Beadle)
on Oct 14, 2001 at 02:20 UTC ( #118701=perlquestion: print w/ replies, xml ) Need Help??
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); }
I would like to improve my coding abilities and seek the wisdom of others to guide me. Thanks.

Comment on Improvement suggestions?
Download Code
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";
    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

      Thanks. That is what I was looking for. This looks much more like PERL than some of the other languages that I program in.
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";
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); }
    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);

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

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://118701]
Approved by root
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others contemplating the Monastery: (3)
As of 2014-10-22 03:37 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    For retirement, I am banking on:










    Results (112 votes), past polls