Beefy Boxes and Bandwidth Generously Provided by pair Networks
Syntactic Confectionery Delight
 
PerlMonks  

Re: Seek Critique of SFTP program, format, structure, overall "Perl"ness

by daddyefsacks (Pilgrim)
on Dec 07, 2003 at 04:00 UTC ( [id://312849]=note: print w/replies, xml ) Need Help??


in reply to Seek Critique of SFTP program, format, structure, overall "Perl"ness

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);
why not:
if (!$interactive) { do_this(); and_this(); and_this_too(); }

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 )
Does that work? From looking at the source, put returns undef on failure:
if ($sftp->put()) { # success } else { # fail }

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" );
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 } }

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!

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: note [id://312849]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others making s'mores by the fire in the courtyard of the Monastery: (3)
As of 2024-04-26 00:51 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found