Re: VarStructor 1.0

[Steve_p]

After seeing all the negative comments and downvotes on this node, I thought I had finally seen the end of it. Unfortunately, the verbal combat regarding this node has spread elsewhere in the Monestary. However, one comment caught my eye. The claim that only one bug had been found amazed me. "Could that code only have one bug?" I thought to myself. So, I decided that I would clear my head of the other comments and the invective that has surrounded this node and take an objective look at the code.

The first thing that troubles me is that use strict and use warnings is no where to be seen. Was this an error of ommission in the paste? So, I took the code from the node and ran it with just warnings. Already, the one bug had been joined by two more. Here's the output.

Name "main::Simple_Arr" used only once: possible typo at VARSTRUCTOR.p
+l line 15.
Name "main::Simple_Var" used only once: possible typo at VARSTRUCTOR.p
+l line 14.
Name "main::hash1" used only once: possible typo at VARSTRUCTOR.pl lin
+e 11.
Use of uninitialized value in pattern match (m//) at VARSTRUCTOR.pl li
+ne 66.
Use of uninitialized value in concatenation (.) or string at VARSTRUCT
+OR.pl line 111.
$Simple_Var = simple
@Simple_Arr = simple1 simple2

%hash1
key1=value1
key2=value2

%hash2
key1=value1
key2=value2

The first three warnings are bogus since that's simply how the program works. The next two warnings are a question to me. Are the real errors or should the program be checking for undef in a few extra places. Anyway, since most programmers consider warning free code to be essential, I'll consider these to be bugs. This took me about 2 minutes to find these two bugs.

I thought for a moment now about adding use strict, but since there are no mys anywhere in the program, I thought I save this for now.

The next big issue I noticed was that of style. All languages have a certain style that is generally accepted within the code. Perl is not an exception to that. Some of the style issues include: all uppercased function and variable names (typically these are all lowercase and constants are the only items that are all uppercased); lack of Pod for documentation; documenting the functionality of a function inside of it (this is typically done separately); quoting variables on assignment (i.e. $Targ = "$_[2]";) is unnecessary; there are scalars and arrays with the exact same name with the difference being the "$" and "@"; it doesn't use warnings; it doesn't use strict; it doesn't use my; and, comments and code go off the right hand side of the screen. On there own, these would be turn offs to your code. Together, it invokes rather violent reactions in people.

Now, that that's over with, a look at the functionality is in order. So, I turn to the comments. The comments are not the greatest, but I finally understood what the function was supposed to do. What concered me then was the API of the function. This function has two rather distinct sets of functionality based on parameters passed into the function. My suggestion is rather than changing the functionality based on a parameter is instead have two separate functions. The code with then be much cleaner and easier to understand so that the " if $Function =~ /^clear$/i and  if $Function =~ /^show$/i are not required throughout the code. The bigger question I have regarding the show functionality is what's wrong with Data::Dumper. The show functionality does something similar but the API to do it stinks.

So, now, I did what I put off at the start. I added use strict and added my to the variables. I did run into the while loop that had comments in the middle of the conditions. That annoyed me a lot. Finally, I got mys in front of all the variables. So, I ran the program, and...

syntax error at VARSTRUCTOR.pl line 12, near "$hash2{"
Execution of VARSTRUCTOR.pl aborted due to compilation errors.

So, I looked at the code and realized that%hash2 had values being assigned to it before being declared. So, I changed the start to

my %hash1=("key1"=>"value1","key2"=>"value2");
my %hash2 = ();
$hash2{"key1"}="value1";
$hash2{"key2"}="value2";
my $Simple_Var = 'simple';
my @Simple_Arr = ('simple1','simple2');

and ran again. Here's the new output is...

Use of uninitialized value in pattern match (m//) at VARSTRUCTOR.pl li
+ne 67.
Use of uninitialized value in concatenation (.) or string at VARSTRUCT
+OR.pl line 116.
$Simple_Var = simple
@Simple_Arr = simple1 simple2

Now, %hash1 and %hash2 are not displayed. More bugs. My guess is that the Regexp that extracts the hashes doesn't like my very well, but that's only a guess.

Now, here's my parting opinon. reset is deprecated because using my and limiting the scope of your variables makes for better programs. The code above one entrenches my belief that use strict and use warnings makes your code more robust than without it. As far as the coding style, this is the kind of code that gives Perl a bad name. It follows almost no traditional coding style. Its API is barely comprehensible. The "show" functionality looks at values of variables as they are declared, not point in time making it nearly useless if your variables are passed to functions. So, in conclusion, I see no use for this code if you make use of current coding standards and use Data::Dumper. It also has a few bugs that may cause this to work incorrectly.