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!
Re: Repeating Code - there has GOT to be a better way!
by BrowserUk (Patriarch) 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.
| [reply] [d/l] |
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"
| [reply] [d/l] [select] |
Re: Repeating Code - there has GOT to be a better way!
by CountZero (Bishop) on Mar 31, 2010 at 19:02 UTC
|
| [reply] [d/l] [select] |
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.
| [reply] [d/l] |
|
| [reply] |
|
Nope, works fine with Perl 5, and has nothing to do with my signature :-)
Perl 6 - links to (nearly) everything that is Perl 6.
| [reply] |
|
|
Re: Repeating Code - there has GOT to be a better way!
by toolic (Bishop) 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.
| [reply] |
|
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.
| [reply] |
|
You might try something like:
my %Cmd;
{
no strict 'vars'; # you have a 'use strict;' line, right?
%Cmd = do ("db/$name" . 'Cmd.pm');
}
| [reply] [d/l] |
|
|
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. | [reply] [d/l] [select] |
|
| [reply] |
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. | [reply] [d/l] |
|
| [reply] |
Re: Repeating Code - there has GOT to be a better way!
by Your Mother (Archbishop) 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.
| [reply] [d/l] |
|
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!
| [reply] |
|
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
| [reply] [d/l] [select] |
|
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.
| [reply] |
|
|
|