Beefy Boxes and Bandwidth Generously Provided by pair Networks
good chemistry is complicated,
and a little bit messy -LW
 
PerlMonks  

Creating Variables Just to Pass into Subroutine?

by mdskrzypczyk (Novice)
on Jul 21, 2015 at 20:38 UTC ( [id://1135711]=perlquestion: print w/replies, xml ) Need Help??

mdskrzypczyk has asked for the wisdom of the Perl Monks concerning the following question:

Hi guys this is my first post and I've only recently started learning perl so please be gentle. I'm not new to programming but am currently working at a company that uses perl to automate some financial business, my job is to refactor old code because some of it is outdated/not used anymore or could look a little more friendly to the user. My question is with the following code snippet:
$who = "su_and_it"; $subject = "Error processing $filename"; $severity = 3; $message = "Server: $server\n"; $message = $message."File: $origfile\n"; $message = $message."Error: Departments missing from WWMBR_NAMES.TXT"; $othercontact = "None"; $path = $work_dir; $file = $missingdeptfilename; &ampNotify($othercontact,$who, $severity,$subject,$message,$path,$file +);
Does it make sense to create these variables and immediately pass them into this subroutine right after? Or should I change the subroutine arguments to just be the values themselves? I feel like creating the variables prior calling the subroutine clutters the code (note that this snippet appears all over the entire set of perl automation but with a few interchanged arguments). Do you have any tips on how to approach this problem? Also, I would greatly appreciate any tips on refactoring perl code in general, methodologies or processes that you may have used before to approach it. I ask because it'll help me develop good scripting practice and also because this is slightly overwhelming to just tackle. Thanks for any help!

Replies are listed 'Best First'.
Re: Creating Variables Just to Pass into Subroutine? (named arguments)
by LanX (Saint) on Jul 21, 2015 at 21:18 UTC
    This looks like pretty old style (aka perl 4) code.

    You should use my in front of variables and you don't need & to call functions anymore.

    To answer your question, the real problem are the many positional parameters you need.

    In this case "Throw away" variables help documenting your code and improve readability.

    BUT the far better solution would be to pass named arguments.¹

    edit

    e.g.

    ampNotify( who => "su_and_it", subject => "Error processing $filename", severity => 3, othercontact => "None", path => $work_dir, file => $missingdeptfilename, message => <<"__message__"); Server: $server File: $origfile Error: Departments missing from WWMBR_NAMES.TXT __message__

    Please note that the use of a here-doc for the message is not essential for this solution, you are also free to write something like

    ampNotify( ... message => qq(Server: $server File: $origfile Error: Departments missing from WWMBR_NAMES.TXT), othercontact => "None", ... );

    TIMTOWTDI, Perl gives you the freedom to chose the best readable/maintainable form for your team, even an extra $message variable can be a good choice here.

    Cheers Rolf
    (addicted to the Perl Programming Language and ☆☆☆☆ :)
    Je suis Charlie!

    UPDATE

    changed the order of arguments, since Here doc must immediately follow the opening << line.

    Thanks to choroba for messaging me. :)

    ¹) example

      Thanks for the input, yes this code is going on about 15 years old at the least... I have a question about the way the arguments are being passed, it looks a lot like hash notation found in perl so I'm confused about how the arguments get interpreted on the subroutine side. Does this implementation require a change in the subroutine itself or will it still be able to read the same arguments that are passed in, meaning that this is only a change to the way the subroutine is called?
        I know it's confusing but => is not a hash notation (ie not exclusively) like : in JS or python.

        it's just a list separator, see "fat comma". I.e. you are passing a list.

        There are two ways to pass hashes , either as a flat list and you have to copy @_ later to a %arg hash. (See example link)

        or as a anonymous hash in curlies and you have to read the hash ref from $_[n] (see my other post)

        HTH ! =)

        Cheers Rolf
        (addicted to the Perl Programming Language and ☆☆☆☆ :)
        Je suis Charlie!

Re: Creating Variables Just to Pass into Subroutine?
by hexcoder (Curate) on Jul 21, 2015 at 21:34 UTC
    Hello,

    refactoring inherited code can be scary, unless you have a good set of tests for all the use cases the code needs to handle. Think of it as a safety net before you start. Also, a version control system would be useful in case of unintentional destructive changes...

    So this should be the foundation to build on and the first step.

    Once you have that, I would start refactoring with the help of Damian Conways' book 'Perl best practices' and the Perl-Critic tool. Keep the amount of changes small and focused, and retest often. Always before checking into version control.

    Regarding your code snippet: assigning one variable to another one at least has the benefit of having more meaningful variable names (which makes it more user friendly, but is a bit redundant). Why not use good names in the first place?

    Passing subroutine parameters in a hash, where the keys name the parameters, can also yield readable code, since the names are part of the subroutine interface then.

    I hope this helps a bit.

      It helps a lot thanks for the reply, I've asked my higher up to prepare a set of test cases for the typical files that will be flowing through the automation for me so that I could make incremental changes and test that the functionality is still the same. I will look into version control, the problem is that these automation files are on a separate server and I'm required to work on windows...so I sftp the files to my work computer and then sftp them back after making changes. Do you have a suggestion on how I could get the version control somehow in here? Also my concern is that version control puts the files online and I'm not sure if the company would be okay with potentially confidential information being stored online...

        Don't use a server on the internet for your repo?
        Just run the service locally.

        The only thing you'll be missing is the off-site storage, and your IT department can handle that.

Re: Creating Variables Just to Pass into Subroutine?
by jeffa (Bishop) on Jul 21, 2015 at 21:37 UTC

    Since your job is to refactor, i recommend using named parameters with defaults:

    sub ampNotify { my %args = @_; # set defaults $args{severity} = defined $args{severity} ? $args{severity} : 3; $args{othercontact} ||= 'None'; .... }

    And then you can call like so:

    ampNotify( message => join( "\n", "Server: $server", "File: $origfile", "Erro +r: foo" ), subject => "Error processing $filename", who => 'su_and_it', path => $work_dir, file => $missingdeptfilename, );

    jeffa

    L-LL-L--L-LL-L--L-LL-L--
    -R--R-RR-R--R-RR-R--R-RR
    B--B--B--B--B--B--B--B--
    H---H---H---H---H---H---
    (the triplet paradiddle with high-hat)
    
      While I do agree that the calling of the function gets cleaned up a lot, I fear that it adds complexity to the function definition that the team in my department may have trouble understanding further down the road. I know I said I need to refactor but I'm also trying to make the code a little more intuitive for the people that don't really get perl that much, is there a way to consolidate the two?
Re: Creating Variables Just to Pass into Subroutine? (refactoring)
by LanX (Saint) on Jul 21, 2015 at 23:14 UTC
    > Also, I would greatly appreciate any tips on refactoring perl code in general, methodologies or processes that you may have used before to approach it.

    here a way to convert ampNotify to allow named parameters without breaking the old positional interface:

    use strict; use warnings; use Data::Dump qw/pp/; sub ampNotify { my ($who ,$subject ,$severity ,$message , $othercontact ,$path ,$f +ile) = @_; # --- this will use named parameters if first arg is hashref if (ref($who) eq "HASH"){ # - defaults (optional) my %defaults = ( who => "su_and_it", severity => 3, othercontact => "None", ); my %args = ( %defaults, %$who); # - parameter check (optional) my @obligatory = qw/subject message path file/; for (@obligatory){ die "missing '$_' argument in ampNotify() " unless exists +$args{$_} } ($who ,$subject ,$severity ,$message , $othercontact ,$path ,$ +file) = @args{qw/who subject severity message othercontact path fil +e/}; } # check content pp [($who ,$subject ,$severity ,$message , $othercontact ,$path ,$ +file)]; # --- your orig code following # ... } # --------- USAGE my $message= <<"__message__"; Server: SERVER File: ORIGFILE Error: Departments missing from WWMBR_NAMES.TXT __message__ ampNotify( { who => "su_and_it", severity => 3, othercontact => "None", path => 'FOO', file => 'BAR', subject => "Error processing 'BAZ' ", message => $message, } );

    Cheers Rolf
    (addicted to the Perl Programming Language and ☆☆☆☆ :)
    Je suis Charlie!

Re: Creating Variables Just to Pass into Subroutine?
by GotToBTru (Prior) on Jul 21, 2015 at 20:42 UTC

    Are any of them used later in the program? That would be the obvious case for when you would want to use a variable instead of a literal. Otherwise, I tend to agree with you. It does clutter things.

    Other advice - if you will be changing these programs, you will want to explore creating tests for them.

    Dum Spiro Spero
      for (0..100) { print "if you will be changing these programs, you will want to expl +ore creating tests for them.\n" }
      ...especially for financial stuff.
      These are pretty much used only right when calling the subroutine, the Notify subroutine typically occurs at end of the line branches of the file where there are many conditional statements to get there so I don't believe that more than one call to Notify or use of these variables is used in more than one run of the script.
Re: Creating Variables Just to Pass into Subroutine?
by Anonymous Monk on Jul 21, 2015 at 23:02 UTC
    This code is o-l-d ... what other Perl-version difference issues is the OP likely to run into?
Re: Creating Variables Just to Pass into Subroutine?
by sundialsvc4 (Abbot) on Jul 22, 2015 at 11:29 UTC

    If it were me, I wouldn’t care if arguments were put into variables ... as long as those variables were local, not global, and as long as they were declared.   This program is obviously very old.   If it has a large set of global variables, those variables create “coupling” all over the place.   If the variables are local, it actually might improve “at-a-glance readability” to use variables because each call to ampNotify() can be desk-checked visually.

    I would start with the basics:   adding use strict; use warnings; to the top of each module and checking for resulting compile-errors.   Arrange the program into subroutines, or even in-line blocks, in which variables are local and declared with my.

    Start now thinking about test-cases, e.g. Test::Most, by which you can verify what the program is doing now, at various levels, and ensure that the code does not “regress” (break ...) as you make changes to it.

    Finally, get to know git, a version-control system that can work within a single directory or set of directories.   Start by creating a repository out of the current directory and check-in the unchanged program.   As you make each change, create a branch to make the change in, then merge the change back into the master-branch.   Use a source-code editor of your choice with built-in support for git.   In this way, you not only preserve a record for yourself of what changes you made and why, but you can also reliably revert the code to any prior state ... and “un-revert it again,” if you wish.   Even if you are the one-and-only person who is working on the code, it’s a great tool to use.

        I'm afraid you might be suffering a chronic case of low level buzzword toxicity.

        There are good points in the core message, but the actualizing of the narrative leaves potential for process improvements.

        Why? Do you recommend global variables? Do you recommend not using strict and/or warnings? Do you recommend not testing? Do you not recommend version control?

        You need to check yourself, BullyUk. This is one time when your hatred is unfounded and unwanted.

        READ FIRST!

      S-Dial verbiaged:

      ..git...create a branch to make the change in, then merge the change back int.o the master-branch.

      Why? And what do I do with the branch afterward?

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://1135711]
Front-paged by GotToBTru
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others imbibing at the Monastery: (7)
As of 2024-03-28 19:50 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found