Beefy Boxes and Bandwidth Generously Provided by pair Networks
Welcome to the Monastery
 
PerlMonks  

Repeating Code - there has GOT to be a better way!

by jedikaiti (Friar)
on Mar 31, 2010 at 16:58 UTC ( #832090=perlquestion: print w/ replies, xml ) Need Help??
jedikaiti has asked for the wisdom of the Perl Monks concerning the following question:

Today's mission, if you choose to accept it, is to help me deal with some very repetitive code. I have a script that works beautifully, but has a problem. It repeats this code about 11 times:

our %<name>Cmd; do 'db/<name>Cmd.pm'; $ApidOffset = $Application{<name>}{AppApidOffset}; foreach $cmd (keys %<name>Cmd) { if (int($fssbCmd{$cmd}{fixed_pattern}) <= 15) { $opcode = sprintf("0%x",$<name>Cmd{$cmd}{fixed_pattern}); } else { $opcode = sprintf("%x",$<name>Cmd{$cmd}{fixed_pattern}); } $apid = sprintf("%x",$ApidBase+$ApidOffset); $cmdData{$cmd} = {opcode => "0x$opcode", apid => "0x$apid"}; };

Wherever you see <name>, that indicates where the code differs each time. I have a bunch of files with the naming convention <name>Cmd.pm, and each file contains a hash called <name>Cmd. In each case, I need to get at the array in the file and do the exact same thing to it.

So I have this code repeating right now, once for each file. I know there has to be a better way, but all I'm coming up with so far is naming a variable with a variable, which seems generally a bad idea, and I'm not too sure it would work anyway.

So here I am, dear Monks - hoping someone knows a better way. A way that will work without repeating the same code 11 times. A way that won't induce heavy drinking and swearing in anyone working with this code after me.

Also, here's an example of the type of data that is being sucked in from the <name>Cmd.pm files:

%<name>Cmd = ( 'command_1' => { _discussion => ' some text about command_1', description => ' shorter text about command_1', safety_level => 'SAFE', fixed_pattern => '143', constraints => undef, build_types => [ 'FLIGHT' ], cmd_id => 250033, target => 'command_target', Command_maps => [] }, 'command_2' => { _discussion => ' some text about command_2', description => ' shorter text about command_2', safety_level => 'SAFE', fixed_pattern => '144', constraints => undef, build_types => [ 'FLIGHT' ], cmd_id => 250014, target => 'command_target', Command_maps => [] } );

As always, THANK YOU!

Kaiti
Swiss Army Nerd

Comment on Repeating Code - there has GOT to be a better way!
Select or Download Code
Re: Repeating Code - there has GOT to be a better way!
by toolic (Chancellor) on Mar 31, 2010 at 17:09 UTC
    If you are open to a completely different approach, I suggest you store your data as data rather than storing your data as code. For example, if you store your data as something standard, such as XML, then you can read it using a standard parser (such as XML::Twig). Then, you don't have to worry about including 11 hash variables via do.

      I wish I could. Alas, at this point, I have no say in how the data is pulled from the DB and stored. Although I might get to re-write that code later, in which case I will keep this in mind! But for now, this is what I am stuck with.

      Kaiti
      Swiss Army Nerd
        You might try something like:
        my %Cmd; { no strict 'vars'; # you have a 'use strict;' line, right? %Cmd = do ("db/$name" . 'Cmd.pm'); }
Re: Repeating Code - there has GOT to be a better way!
by moritz (Cardinal) on Mar 31, 2010 at 17:17 UTC
    and each file contains a hash called <name>Cmd.

    So you either need symbolic references, or (preferred) a hash containing references to all these hashes, and then simply loop over <name> and look into that hash.

    Perl 6 - links to (nearly) everything that is Perl 6.

      Is this going to require Perl 6? I ask because I'm on 5.10.1.

      Kaiti
      Swiss Army Nerd
        Nope, works fine with Perl 5, and has nothing to do with my signature :-)
        Perl 6 - links to (nearly) everything that is Perl 6.
Re: Repeating Code - there has GOT to be a better way!
by brap (Pilgrim) on Mar 31, 2010 at 17:32 UTC
    Not that this answers your question, but you might be able to replace

    if (int($fssbCmd{$cmd}{fixed_pattern}) <= 15) { $opcode = sprintf("0%x",$<name>Cmd{$cmd}{fixed_pattern}); } else { $opcode = sprintf("%x",$<name>Cmd{$cmd}{fixed_pattern}); }

    with

    $opcode = sprintf("%02x",$<name>Cmd{$cmd}{fixed_pattern});

    It might help a little bit, anyway.

      Always happy to get suggestions to improve my code, and every little bit helps! Thanks!

      Kaiti
      Swiss Army Nerd
Re: Repeating Code - there has GOT to be a better way!
by jethro (Monsignor) on Mar 31, 2010 at 17:33 UTC

    Is the code in the pm files fixed? Or generated by some other script? Or handwritten? If it is fixed I don't see anything better than to use symbolic references (or use eval which is not that different from using a symbolic ref in this case). But you could at least confine it to a small part of your code and put warning signs around it (this is totally untested code and sure to contain bugs) :

    my $Cmdref; #---------- # Using symbolic references to get code files into name space no strict refs; eval "our %${name}Cmd; do 'db/${name}Cmd.pm'; $Cmdref= \%${name}Cmd;"; use strict refs; #----------- ... foreach $cmd (keys %$Cmdref) { ... $opcode = sprintf("0%x",$Cmdref->{$cmd}{fixed_pattern});

    But if the data in those files could look different you have a lot more options. You could change it to look like a real module and use the core-module Exporter to get the hashes into your namespace. Or simply have some code that adds a reference to each hash into a global array:

    my %ThingyCmd= ... push @main::allCmd,\%ThingyCmd;

    Or you could create files that conform to Storable, JSON or to the Data::Dumper file format and use the corresponding CPAN modules to read them in. I believe Data::Dumper calls that "thaw"

Re: Repeating Code - there has GOT to be a better way!
by cleverett (Friar) on Mar 31, 2010 at 17:43 UTC
    You need to make it so you can run the same code for every one of the files. The thing that changes every time is %<name>Cmd.

    One question to ask is: must all the %<name>Cmd hashes in the .pm files have different names?

    If yes, you need alias %<name>Cmd onto %Cmd or something like that. This is kind of cool, but not recommended to do routinely.

    If no, then just rename the %<name>Cmd hashes in each of the .pm files to %Cmd.

    Either way, you can process all the files in a loop or a subroutine (or both) with code that looks something like this:

    our %Cmd; do "db/$name" . 'Cmd.pm'; $ApidOffset = $Application{$name}{AppApidOffset}; foreach $cmd (keys %Cmd) { if (int($fssbCmd{$cmd}{fixed_pattern}) <= 15) { $opcode = sprintf("0%x",$Cmd{$cmd}{fixed_pattern}); } else { $opcode = sprintf("%x",$Cmd{$cmd}{fixed_pattern}); } $apid = sprintf("%x",$ApidBase+$ApidOffset); $cmdData{$cmd} = {opcode => "0x$opcode", apid => "0x$apid"}; };
    Hope this helps.

      I have no control over what is in the .pm files, so I can't make them all the same name.

      Thanks!

      Kaiti
      Swiss Army Nerd
Re: Repeating Code - there has GOT to be a better way!
by Your Mother (Canon) on Mar 31, 2010 at 17:57 UTC

    Tangent: danger sign-

    foreach [where is the my?] $cmd (keys %<name>Cmd) {

    You should always be using strict. It will save you hours and hours leading to days and even weeks of effort over time.

      Yes, I am using strict, and the "my $cmd;" line actually appears further up the code. Unless there's a good reason not to, in which case I will happily change it!

      Kaiti
      Swiss Army Nerd

        Great. It's best to restrict it to the smallest possible scope. Having it at the top makes it, more or less, a global which can be seen or persist anywhere in the script. This can lead to lame/long debugging sessions. Better to put it inline with the loop.

        There is an excellent reason why you should always use my to declare a for loop variable in the loop - a for loop variable isn't what you think it is! Consider:

        my $loopVar = 10; for $loopVar (1 .. 5) { print "$loopVar\n"; } print $loopVar;

        What do you expect to see printed? What do you actually see printed? Now try this:

        my $loopVar = 10; my @values = (0 .. 4); for $loopVar (@values) { ++$loopVar; } print "$loopVar, @values";

        What do you expect to see printed? What do you actually see printed?

        A for loop variable is aliased to each element of the for list in turn. When you use a previously declared variable as the loop variable you are really only using the name, the value is untouched.

        So, always declare your for loop variable in the loop header because it ain't what you think it is anyway and declaring it make sure everyone gets that idea.

        Note that a C style for loop is different and there it does often make sense to use a variable from the scope global to the loop.


        True laziness is hard work
Re: Repeating Code - there has GOT to be a better way!
by BrowserUk (Pope) on Mar 31, 2010 at 18:29 UTC

    As others have said, use a hash rather than symbolic references. Here's how:

    #! perl -slw use strict; use Data::Dump qw[ pp ]; my %cmds; for my $fname ( <*Cmd.pm> ) { print $fname; my( $name ) = $fname =~ m[(^.+)Cmd.pm]; %{ $cmds{ $name } } = do $fname; } pp \%cmds; __END__ C:\test>832090.pl billCmd.pm fredCmd.pm { bill => { command_1 => { Command_maps => [], _discussion => "\n some text about command_1", build_types => ["FLIGHT"], cmd_id => 250033, constraints => undef, description => "\n shorter text about command_1 +", fixed_pattern => 143, safety_level => "SAFE", target => "command_target", }, command_2 => { Command_maps => [], _discussion => "\n some text about command_2", build_types => ["FLIGHT"], cmd_id => 250014, constraints => undef, description => "\n shorter text about command_2 +", fixed_pattern => 144, safety_level => "SAFE", target => "command_target", }, }, fred => { command_1 => { Command_maps => [], _discussion => "\n some text about command_1", build_types => ["FLIGHT"], cmd_id => 250033, constraints => undef, description => "\n shorter text about command_1 +", fixed_pattern => 143, safety_level => "SAFE", target => "command_target", }, command_2 => { Command_maps => [], _discussion => "\n some text about command_2", build_types => ["FLIGHT"], cmd_id => 250014, constraints => undef, description => "\n shorter text about command_2 +", fixed_pattern => 144, safety_level => "SAFE", target => "command_target", }, }, }

    Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
    "Science is about questioning the status quo. Questioning authority".
    In the absence of evidence, opinion is indistinguishable from prejudice.
Re: Repeating Code - there has GOT to be a better way!
by CountZero (Bishop) on Mar 31, 2010 at 19:02 UTC
    I see two possible solutions:
    1. Although you cannot change the files, nobody says you have to use them "as is": you could pre-process them and turn them into a more workable data-format and then work with this data; or
    2. You could write a subroutine that takes two arguments: the first is the <name> and the second is the %<name>Cmd hash:
      sub process { my ($name, %Cmd) = @_; $ApidOffset = $Application{$name}{AppApidOffset}; foreach $cmd (keys %Cmd) { if (int($fssbCmd{$cmd}{fixed_pattern}) <= 15) { $opcode = sprintf("0%x",$Cmd{$cmd}{fixed_pattern}); } else { $opcode = sprintf("%x",$Cmd{$cmd}{fixed_pattern}); } $apid = sprintf("%x",$ApidBase+$ApidOffset); $cmdData{$cmd} = {opcode => "0x$opcode", apid => "0x$apid"}; }; }
      Of course you will still have 11 subroutine calls to make, but I think you can survive that!

    Update: fixed a typo in the subroutine (%Cmd instead of %cmd)

    CountZero

    A program should be light and agile, its subroutines connected like a string of pearls. The spirit and intent of the program should be retained throughout. There should be neither too little or too much, neither needless loops nor useless variables, neither lack of structure nor overwhelming rigidity." - The Tao of Programming, 4.1 - Geoffrey James

Log In?
Username:
Password:

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

How do I use this? | Other CB clients
Other Users?
Others having an uproarious good time at the Monastery: (7)
As of 2014-08-01 00:46 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My favorite superfluous repetitious redundant duplicative phrase is:









    Results (256 votes), past polls