anomilie:

You're definitely making things far too difficult for yourself. This isn't a comprehensive review, since I have to go to work soon, but

The first thing I noticed is that your code is difficult to read. Several factors contribute to this:

• Your indentation isn't consistent. Proper indentation makes it easier to read throughout your code and get a feel for how it's structured. I suggest you use an editor to help you keep your code formatted, or at least use perltidy to clean up the indentation whenever you start having trouble reading it.
• You've placed 'next' statements in too many places. Normally 'next' is a special case, rather than the typical case, so an experienced programmer will have to read and reread your code multiple times to find what's happening. (I'll revisit this point in a moment.)
• You're duplicating code too often, especially in both branches of if/then statements. (I'll revisit this one, too.)

It turns out that the last two points are related. Any time you have code like this:

if (some condition) {
   ... some code ...
   $precords{$product_key}{"total_count"}++; # BOTH BRANCHES
   ... more code ...
   next;                                     # BOTH BRANCHES
}
else {
   ... some code ...
   $precords{$product_key}{"total_count"}++; # BOTH BRANCHES
   ... more code ...
   next;                                     # BOTH BRANCHES
}
[download]

See those lines labelled 'BOTH BRANCHES'? Since it's the same code, and nothing else in the 'more code' chunks affect those lines in both branches, you can move those statements to a more convenient location, like this:

$precords{$product_key}{"total_count"}++;
if (some condition) {
   ... some code ...
   ... more code ...
}
else {
   ... some code ...
   ... more code ...
}
next;
[download]

I moved the 'next' statement until after the if/then statement, since we need to be sure that we execute the code. I put the other statement before the if/then statement because nothing in the if/then statement used that value.

You want to avoid duplicating code, because it makes the code harder to read:

• You use branching because you want to do different actions. Putting identical bits of code in both branches is confusing.
• It makes your code longer.

So when maintaining the code, you're making the programmer have to read more code, try to figure out why the same statements are being used in multiple places, all for no real benefit. Another problem with duplicated code is that if you have a "quick fix" you want to make and you make the fix, it may have only been a partial fix, as you have the *same* code duplicated in other places that need the change. By avoiding duplication, you avoid the chance of making a partial fix.

Now let's take a look at your 'next' statements. Generally, they're used for special cases, but in your code, they're used so often that anyone making a change to your code will have to read and re-read the code frequently to see what it's doing. It can make the code pretty difficult to read.

By repeatedly moving code duplicated in both branches of your if/then statements, your code boils down to something like this:

for {
   if (value == '1') {
      ... do stuff ...
      next;
   }
   if (value == '0') {
      ... do stuff ...
     next;
   }
   ... do nothing ...
}
[download]

And since the value isn't changing in any of the code in your loop, the next statements don't do anything at all, so you could simplify it a little more to:

for {
   if (value == '1') {
      ... do stuff ...
   }
   elsif (value == '0') {
      ... do stuff ...
   }
}
[download]

Note: I changed the if to elsif to let the programmer know that you can't execute both conditional blocks. In your original code, if you changed something in the first block to turn value to '0', then suddenly your program would start always executing the code in the second if statement, too!

Finally, using == with strings is a recipe for bugs. If you want to compare numbers use ==, for strings use 'eq'. So statements like:

if ($comboHashRef{$key}->[0] == '0')
[download]

should be changed to either:

if ($comboHashRef{$key}->[0] == 0)
[download]

if you want to compare the value as a number, or:

if ($comboHashRef{$key}->[0] eq '0')
[download]

if you want to compare it as a string. If you're reading the records as strings from a file and parsing it, I'd suggest comparing it as a string.

I wanted to go over some more stuff, like comments and variable names, but I ran out of time. Before I started this post, I noticed that others were already offering some comments, so I'll leave further comments to others...

...roboticus

When your only tool is a hammer, all problems look like your thumb.

---

---

Posts are HTML formatted. Put <p> </p> tags around your paragraphs. Put <code> </code> tags around your code and data!

Titles consisting of a single word are discouraged, and in most cases are disallowed outright.

Read Where should I post X? if you're not absolutely sure you're posting in the right place.

Please read these before you post! —

• How do I compose an effective node title?
• How do I post a question effectively?
• Markup in the Monastery

Posts may use any of the Perl Monks Approved HTML tags:

a, abbr, b, big, blockquote, br, caption, center, col, colgroup, dd, del, details, div, dl, dt, em, font, h1, h2, h3, h4, h5, h6, hr, i, ins, li, ol, p, pre, readmore, small, span, spoiler, strike, strong, sub, summary, sup, table, tbody, td, tfoot, th, thead, tr, tt, u, ul, wbr

You may need to use entities for some characters, as follows. (Exception: Within code tags, you can put the characters literally.)

	For:		Use:
	&		&
	<		&lt;
	>		&gt;
	[		&#91;
	]		&#93;

Link using PerlMonks shortcuts! What shortcuts can I use for linking?

See Writeup Formatting Tips and other pages linked from there for more info.