Beefy Boxes and Bandwidth Generously Provided by pair Networks
Pathologically Eclectic Rubbish Lister
 
PerlMonks  

Re: looking for feedback on my first script

by shmem (Chancellor)
on Jan 08, 2021 at 16:19 UTC ( #11126614=note: print w/replies, xml ) Need Help??


in reply to looking for feedback on my first script

Looks good. If only my first script was so orderly...

Some things I'd do different:

my $Usage = "usage: volume root_dir subvol timeframe dateformat keep\n +"; @ARGV == 6 or die $Usage; # we need six arguments my ( $subvol_to_snapshot_arg, # this $snap_root_dir_arg, # assigns all variables $yabsm_subvol_name_arg, # in one go $timeframe_arg, # and leaves room $date_format_arg, # to comment $snaps_to_keep_arg, # what they are about ) = @ARGV;

That's a matter of style (and lazyness :-)

I'd use sprintf

sub create_snapshot_name { my ($min, $hr, $day, $mon, $yr) = (localtime())[1..5]; $mon++; # month count starts at zero. $yr += 1900; # year represents years since 1900. my @fields = $date_format_arg eq 'mm/dd/yyyy' ? ($mon, $day) : $date_format_arg eq 'dd/mm/yyyy' ? ($day,$mon) : die "$date_format_arg is not a valid date format"; sprintf "day=%02d_%02d_%04d,time=%02d:%02d",@fields,$yr,$hr,$min; }
which obsoletes the pad() subroutine. Expressing the if/else/elsif as a construct of ternaries (COND ? IF_TRUE : IF_FALSE) is, again, a matter of style. The sprintf being the last expression eliminates the need for an explicit return.

edit:

Since the documentation is contained in the script, and it states

This script should not be used by the end user.

you could stick the following directive into it up front

exec "pod2man $0" if -t; # display manual page if invoked from termina +l

which can be overridden redirecting /dev/null into it as STDIN.

perl -le'print map{pack c,($-++?1:13)+ord}split//,ESEL'

Replies are listed 'Best First'.
Re^2: looking for feedback on my first script
by thirtySeven (Acolyte) on Jan 08, 2021 at 16:27 UTC
    the usage tip will make those variable interpolations much more readable. I am indeed gonna switch to sprintf.
Re^2: looking for feedback on my first script
by Marshall (Canon) on Jan 09, 2021 at 16:37 UTC
    A minor quibble with "The sprintf() being the last expression eliminates the need for an explicit return.
    I always put an explicit return except for sort subs. Here the code is going to return the return value of sprintf(), presumably "1" which really doesn't mean anything in the context of this code. Ending with return; statement would return undef. I guess this is a style thing, YMMV.

    BTW, I always go with a YYYY-MM-DD format. For (a) consistency and (b) this produces a natural ASCI sort order that does "what you expect".

      Here the code is going to return the return value of sprintf(), presumably "1" which really doesn't mean anything in the context of this code.

      From the docs (sprintf):

      sprintf FORMAT, LIST
      Returns a string formatted by the usual "printf" conventions of the C library function "sprintf".

      Please elaborate how sprintf might return "1", other than as sprintf "1", as sprintf "%d", 1 or sprintf "%s",1; and how that might be the case given the incriminated statement.

      perl -le'print map{pack c,($-++?1:13)+ord}split//,ESEL'
        Yes, you are correct in that I should have taken the time to look up Perl's exact return value from sprintf(). I fudged the issue by saying "presumably 1" - instead of something that means "true" or the exact reference to a Perl doc. In C the printf function returns the number of characters that are printed. If there is some error then it returns a negative value. There is a "succeed or failed" interpretation of this. But nobody checks the return value of printf(). In Perl, if printf() fails, a fatal error will be thrown long before it returns to the caller. I've never checked the return value of sprintf.

        We are off of my main point!
        The return value of sprintf() means nothing in this context, but it will returned absent an explicit return statement. Why return a string that the caller doesn't care about and means nothing as opposed to returning a more simple undef value? I am simply saying that I prefer having explicit return values. What your post said is "correct". Please do not take this as any real argument. We are talking about a very fine point here, not any real criticism.

        Ok, I see the situation better now.
        I would have written  return sprintf "...."
        That is just a matter of coding style. Not any real "problem".

Log In?
Username:
Password:

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

How do I use this? | Other CB clients
Other Users?
Others contemplating the Monastery: (2)
As of 2021-07-24 20:01 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found

    Notices?