If the major and minor frame indices are integers, are they sequential and could they be stored in array references rather than hash references? This might allow you to shorten/tighten things a bit -- for example, eliminating all the numerical sorting of keys and integer checks.

I'd suggest pulling all your hard-coded numbers into variables that explain them. (E.g. why just minor frame data for 3 .. 17, why all the special logic around minor frame index 3 being 165.)

Also, while "golfing" (reducing characters) can often make things more confusing, in this case, I think trying to strip out some of the "line-noise" of Perl so that the logic is clearer might help. That would include changing %{ $ref_data } to just %$ref_data and $ref_data->{$filename}->{$major_frame_index}->{$minor_frame_­index} to $ref_data->{$filename}{$major_frame_index}{$minor_frame_­index} if not just $ref_data->{$filename}{$i}{$j} (simple variables for loop iterators). You could also break those up into temporary variables for each loop so you're not cluttering up the screen with the long names.

You might consider moving the logic when you're at row length or you hit your special frame data 3 equals 165 actions to separate subroutines with descriptive names. That would strip out lines to make the logic clearer (at the cost of some efficiency, true) as it brings your alternate conditions closer together visually.

Something that jumps out is that you're pushing an array slice onto @cur_spin but checking to see if the length of @cur_spin ever hits the row length exactly. What's implied is that the row length is a multiple of the slice length but that's not immediately obvious. And what happens if you get to the end of a major frame and came up short? It doesn't look like anything would be written.

You also don't seem to sort the keys of the minor frames when you loop through them. Is that a bug? Since just about every hash you have you wind up wanting sorted, you might just want to use a sorted hash in the first place. (Though I haven't used it, CPAN search found Tie::Sorted::Hash.)

Having looked through your code a few times now, it looks like your special logic for @cur_spin equals zero is trying to pick off the name for the row and you need that logic since you reuse @cur_spin as your accumulator and don't want to try to generate a name each time through.

That makes me think you should consider just assembling all of the minor frames up front and just splice them off a row at at time rather than looping over them. For example, I'd approach it more like this:

sub WriteSpinData { my ($OUT_FH, $ref_data) = @_; my $frames_per_row = 65; for my $filename (sort keys %$ref_data ) { my $basename = basename $filename; # create a list of major frames for the file my @majors = map { $ref_data->{$filename}{$_} } sort { $a <=> $b } keys %{ ref_data->{$filename} }; for my $major ( @majors ) { # join all minor frames together my @minors = map { $major->{$_} } sort { $a <=> $b } # assuming sorted grep { /\d+/ } keys %$major; # print them a row at a time while ( my @row = splice( @minors, 0, $frames_per_row ) ) { my $row_name = find_row_name( @row ); my @data = map { @{$_}[3..17] } @row; print $OUT_FH join( q{,}, $row_name, @data ), "\n"; } } print "$basename: " . @majors . " major frames.\n"; } }

This kind of assumes that you can generate a row name in a subroutine from the row itself other than with all those indexes. If you really want/need the indexes, then you'll have to do something more like what you've got with all the indices and looping instead of the way I've done it with map. I don't really have enough context for your application to say.

-xdg

Code written by xdg and posted on PerlMonks is public domain. It is provided as is with no warranties, express or implied, of any kind. Posted code may not have been tested. Use of posted code is at your own risk.


In reply to Re^3: Refactoring nested if block by xdg
in thread Refactoring nested if block by bowei_99

Title:
Use:  <p> text here (a paragraph) </p>
and:  <code> code here </code>
to format your post, it's "PerlMonks-approved HTML":



  • 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! —
  • 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:
    & &amp;
    < &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.