in reply to Script Review...

Just a few stylistic points:

I'll assume that the identing looks goofy because you pasted the code in with a combination of space and tab characters (with indent level set to 4), instead of just spaces -- no big deal here, just make sure the copy you present to the interviewer shows proper indentation.

The nesting of if..else... blocks, and the distance between a short else block from the condition that triggers it, are not severely bad here, but they are unnecessary impediments to readability and maintainability. Your handling of the config file could go like this:

while (<CONF>) { next unless ( /^(.+)\s(http:\/\/\S+)\s+(.+)$/ ); # just skip unusa +ble lines my ( $NAME, $URL, $ALT ) = ( $1, $2, $3 ); $http->reset; if ( $http->request( $URL ) != 200 ) { warn "cannot read from $URL ($NAME)\n"; next; } if ( http->body() =~ /.../ ... ) { my $IMGURL = $1; ... $http->reset; if ( $http->request( $IMGURL ) != 200 ) { warn "drat! tried to get $IMGURL (for $NAME) but couldn't\ +n"; next; } # OK, got the image... and so on... } }
If you really want to report on the status of reading the config file (e.g. report unusable lines), just read the file in a short loop, report the results after closing it, then do a loop over the usable lines -- eg:
my %name; my %alt; my %bad; while (<CONF>) { if ( /^(.+)\s(http:\/\/\S+)\s*(.+)$/ ) { $name{$2} = $1; $alt{$2} = $3; } else { $bad{$.} = $_; } } close CONF; if ( keys %bad ) { warn "unusable config data at line $_:\n $bad{$_}" for ( sort { $a <=> $b } keys %bad; } warn "Loading ",scalar( keys %alt )," comic sites\n"; for ( keys %alt ) { # ... and so on }
Note that a "my ... " statement only needs parens when you're doing a list of variables. (update: or when you're declaring a single scalar and assigning a value from something that returns a list, e.g.   my ($first) = sort @array;)

You weren't doing a consistent amount of error-checking and problem-reporting with your different $http->request calls.

You should add pod that describes the app and gives the necessary detail for creating a config file from scratch (or provides the content of a sample config file).

Okay, this is for comics, so I guess the spelling of "$dimention" is supposed to be a joke, right?

Replies are listed 'Best First'.
Re: Re: Script Review...
by allolex (Curate) on Oct 25, 2003 at 23:55 UTC
    Okay, this is for comics, so I guess the spelling of "$dimention" is supposed to be a joke, right?

    Maybe not. :) Look at this:

    (WIN) whatever $ENV{APPDATA} is on your machine (should be under you documents and settings dir under appdata as long as your on NT (emphasis added)

    Often attention to detail gets you the job. I've dealt with a lot of applicants and have discounted minor errors in interviews, but spelling and grammar errors in any part of the written application look really bad (excellent idea to get it checked out at the Monastery, BTW). One way of avoiding this particular problem is to never write a contraction: it is (not it's), they are (not they're), or worse---confusing "their/there/they're". It's not code, but it counts.

    --
    Allolex

    Perl and Linguistics
    http://world.std.com/~swmcd/steven/perl/linguistics.html
    http://www.linuxjournal.com/article.php?sid=3394
    http://www.wall.org/~larry/keynote/keynote.html