in reply to Re^2: Please critique this script -- Read emails and write URLs into bookmarks
in thread Please critique this script -- Read emails and write URLs into bookmarks

I would check the following:

To me, that's basic defensive programming. Check everything -- you never know when Something Weird will happen; it's better that your script captures it rather than the script dying for some unknown reason.

Alex / talexb / Toronto

Thanks PJ. We owe you so much. Groklaw -- RIP -- 2003 to 2013.

Replies are listed 'Best First'.
Re^4: Please critique this script -- Read emails and write URLs into bookmarks
by Anonymous Monk on Oct 07, 2016 at 08:13 UTC

    I light of my brainfart yesterday I felt compelled to show a revised script including proper error handling:

    I blame it on the meds!

    #!/usr/bin/perl

# read emails I sent myself containing just a link
# and make a HTML-redirect based bookmark out of them
#
# SPEC
# read given files as internet messages
# use subject as title and filename, escape as needed
# use one URL in body as href (if there a two they are both the same)
# and output one HTML file for each bookmark

use strict;
use warnings;
use Carp qw(cluck confess);
use Encode;

use Email::Simple;
use HTML::Entities;
use Path::Tiny;
use URI::Find;

use version; our $VERSION = qv('0.0.1');

if ( @ARGV == 0 ) { confess 'Invalid number of arguments' }

foreach my $in_filepath (@ARGV) {

    if ( not path($in_filepath)->exists ) {
        cluck "$in_filepath: Input file '$in_filepath' does not exist"
+;
        next;
    }

    my $in_file = Path::Tiny->new($in_filepath)->slurp
      or confess "$in_filepath: Fatal error in module";
    my $message = Email::Simple->new($in_file)
      or confess "$in_filepath: Fatal error in module";

    if ( not length $message->as_string
        or $message->as_string =~ /\A\s*\z/ )
    {
        cluck "$in_filepath: Invalid message";
        next;
    }
    if ( not length $message->header('Subject') ) {
        cluck "$in_filepath: Invalid subject";
        next;
    }

    my $subject = decode( 'MIME-Header', $message->header('Subject') )
      or confess "$in_filepath: Fatal error in module";
    my $title = HTML::Entities::encode($subject)
      or confess "$in_filepath: Fatal error in module";
    my $out_filepath = $subject =~ s/[%\/\?<>\\:\*\|":]/_/gr . '.URL.H
+TML';

    # TODO IMPR URL::Search might be nicer than URI::Find for some cas
+es
    my @uris;
    my $finder = URI::Find->new( sub { push @uris, $_[0] } );
    $finder->find( \$message->body );

    if ( @uris == 0 ) { cluck "$in_filepath: Invalid number of URIs"; 
+next; }
    if ( path($out_filepath)->exists ) {
        cluck "$in_filepath: Output file '$out_filepath' exists";
        next;
    }

    path($out_filepath)->spew(
"<!DOCTYPE html><title>$title</title><meta http-equiv=\"refresh\" cont
+ent=\"0; url=@uris\">",
    ) or confess "$in_filepath: Fatal error in module";

}

    [download]
[reply]
[d/l]

      Rather than 

        if ( not length $message->as_string
  or $message->as_string =~ /\A\s*\z/ )
{
    cluck "$in_filepath: Invalid message";
    next;
}

        [download]
      I would prefer something more along the lines of this: 
        my @errors;

length $message->as_string or
  push ( @errors, 'Message empty' );
$message->as_string =~ /\A\s*\z/ or
  push ( @errors, 'Invalid message' );

if ( @errors ) {

  foreach my $e ( @errors ) { cluck "in_filepath: $e"; }
  next;
}

        [download]
      Also, I would recommend || to or two conditionals together, rather than or.

      Alex / talexb / Toronto

      Thanks PJ. We owe you so much. Groklaw -- RIP -- 2003 to 2013.

[reply]
[d/l]
[select]
Re^4: Please critique this script -- Read emails and write URLs into bookmarks
by Anonymous Monk on Oct 06, 2016 at 19:28 UTC

    I forgot to check URI::Find->new as well. Thanks for that.

    About the others, maybe I should elaborate my ideology here:

    I usually prefer technical error messages to 'clean' ones. Especially when I'm not perfectly sure where the errors could come from or if I don't have the time to check all odds and ends. The rationale being that my own error messages could only be worse.

    The errors are handled. They produce descriptive (if not overly verbose) messages. But in a script of this nature developer and user are usually quite close. So this verbosity turns out to be very useful.

    However, checking the number of input arguments and checking if $uris is not empty are clearly omissions which I will fix.

[reply]
[d/l]
      Actually $subject (if empty) and the output filepath (if exists) should be checked as well.
[reply]
[d/l]
        Hold the phone, I'm talking out of my ass here. There's actually no error handling going on, only error messages. Yeah, I need error-handling code.
[reply]

In Section Seekers of Perl Wisdom