To me the amount of comments in the code serve to make it less readable. If you need that many comments to allow people to understand your code perhaps you should rewrite the code to be more understandable. Use comments to explain why you did something and let the code should show how it's done. You write my %HoHoHoA; #used in readListOfFilesToSFTP() What does that serve to tell someone reading your code? I'm not sure what exactly but I am sure Santa Claus is somehow involved.

You've defined two constants TRUE and FALSE which aren't used anywhere in the code that I see. Check this page for an explanation of why that's usually a silly thing to do.

There are several blocks of code with a pattern of 

do_this() if (! $interactive);
and_this() if (! $interactive);
and_this_too() if(! $interactive);

[download]
why not: 
if (!$interactive) {
    do_this();
    and_this();
    and_this_too();
}

[download]

If looks like you are trying to see if put succeeds but in a really obscure way.. 

if (
    eval {
        $sftp->put( $file, $target_file, \&callback );
    } ne 0 
)

[download]
Does that work? From looking at the source, put returns undef on failure: 
if ($sftp->put()) {
    # success
}
else {
    # fail
}

[download]

There are several areas where you are doing things like this: 

  writeLog("* * * ABNORMAL COMPLETION * * * Error: $!. Unable to open 
+file $files_to_sftp" );
  print( STDOUT "* * * ABNORMAL COMPLETION * * * Error: $!. Unable to 
+open file $files_to_sftp\n" );

[download]
Why don't you make the writeLog function a little smarter rather than littering the code with all those checks for  if (!debug) and if (! $interactive) maybe set up a closure so that the writeLog function knows what mode it is in and knows what to do with the messages it receives something like: 
sub set_up_logger {
    my ($interactive, $batch, $whatever) = @_;
    sub writeLog {
        my $msg = shift;
 
        if ($interactive) {
            print STDOUT $msg;
        }
        elsif ($batch) {
            print FILE $msg;
        }
        ## whatever whatever
    }
}

[download]

I see some system calls where the return value isn't being checked, bad idea.

Anyway those are just some quick comments I had while reading down through your code, I didn't actually run it or check too hard for bugs. I think overall it could benefit from more design work upfront to make it more modular. Best of luck!

In reply to Re: Seek Critique of SFTP program, format, structure, overall "Perl"ness by daddyefsacks
in thread Seek Critique of SFTP program, format, structure, overall "Perl"ness by mikedshelton

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":


  • Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!
  • Titles consisting of a single word are discouraged, and in most cases are disallowed outright.
  • Read Where should I post X? if you're not absolutely sure you're posting in the right place.
  • Please read these before you post! —
  • Posts may use any of the Perl Monks Approved HTML tags:
    a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr
  • You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)
            For:     Use:
    & &amp;
    < &lt;
    > &gt;
    [ &#91;
    ] &#93;
  • Link using PerlMonks shortcuts! What shortcuts can I use for linking?
  • See Writeup Formatting Tips and other pages linked from there for more info.