Beefy Boxes and Bandwidth Generously Provided by pair Networks
laziness, impatience, and hubris
 
PerlMonks  

Looking for less code

by Sun751 (Beadle)
on Aug 11, 2009 at 06:45 UTC ( [id://787506]=perlquestion: print w/replies, xml ) Need Help??

Sun751 has asked for the wisdom of the Perl Monks concerning the following question:

Below I have while loop which loop through each line at a time, I was wondering if there is any possibility to reduce the lines of code, less code doing same process like below?
while ($line = <DATA>) { if ($line =~ /WNT/ || $line =~ /UNX/) { print "$line\n"; } if ($flag) { if ($target_db ne 'sql') { next if ($line =~ /$target_db/i); } else { next if ($line =~ /informix/i || $line =~ /oracle/i) } print $FH "$line"; if ($line =~ /$target_os/) { undef $flag; next; } } if ($line =~ /UNX/ || $line =~ /WNT/ || $line =~ /COMPONENT/) { if(!(defined $flag)) { $flag = 1; print $FH "$line"; } } }
Cheers!

Replies are listed 'Best First'.
Re: Looking for less code
by moritz (Cardinal) on Aug 11, 2009 at 06:51 UTC
    It seems that all of this fiddling with the $flag could be replaced by a flip-flop operator:
    while (<DATA>) { ... if (/UNX|WNT|COMPONENT/ .. /$target_os/) { ... } }

    See perlop for details on the flip-flop operator.

    But without having a textual description of what your program does it seems hard for me to check if that does what you want.

Re: Looking for less code
by Marshall (Canon) on Aug 11, 2009 at 08:03 UTC
    First, I would improve the indentation: This is probably wrong:
    if ($line =~ /UNX/ || $line =~ /WNT/ || $line =~ /COMPONENT/) { if(!(defined $flag)) { $flag = 1; print $FH "$line"; } }
    In a code like this,
    if ($line =~ /$target_os/) { undef $flag; next; }
    NEVER, repeat NEVER fiddle fiddle around with undef as a "false" value (except in rare cirsumstances). If you mean that $flag should be "false", set $flag to "0 (zero)". A simple rationale is that if you want to print $flag for debugging, "undef" is an error. Another rationale would be that you KNOW that it is "false" not "undefined"!. So say so!

    the above code is hard for me to understand your intent. Can you post some set of data and the "pass" and "fail" cases? That would be most helpful.

      A simple rationale is that if you want to print $flag for debugging, "undef" is an error

      How come? Last I looked it was a warning, and only if warnings are enabled.

      Additionally it's a good idea to use Data::Dumper or something similar for debugging output anyway, so I don't see the point you're trying to make.

      Another rationale would be that you KNOW that it is "false" not "undefined"!. So say so!

      undef is also false - why would 0 be clearer false than undef? 0 is a number to me, not primarily a "false" boolean value.

        Point 1: You should run with both "warnings" and "strict" enabled.

        Point 2: I recommend taking some 'C' classes and you will understand the difference between "undef" and "zero".

        Update: My comment about 'C' was not meant in a derogatory way. I stand by my assertion that code should run with both warnings and strict. Some performance increase can be obtained without warnings in well debugged code. But that was not the point here. There is a difference in general between, I know for sure that this $var is "false": ('',"",0) and I "don't know what the value is", (undef), so I am gonna assume "false".

        Update: minor typo punctuation corrections.

        #!/usr/bin/perl -w use strict; my $x = undef; print "$x\n"; $x = 0; print "$x\n"; #prints: #Use of uninitialized value $x in string at C:\TEMP\perl1.pl line 5. 0
Re: Looking for less code
by mzedeler (Pilgrim) on Aug 11, 2009 at 10:38 UTC

    The comment by JavaFan provides a very short version, but that aside, having a variable named $flag is a really bad idea. Usually you use the name to describe the purpose of the variable, not the type. Otherwise you'll end up with names like $string, $string2, $number, $numbers and so forth. Knowing what you want $flag to do could in this case help simplifying your code even further.

Re: Looking for less code
by ELISHEVA (Prior) on Aug 11, 2009 at 11:41 UTC

    Before worrying about the amount of code, I'd first focus on making your code more idiomatic and self-documenting. Sometimes it takes more code, not less, to write good code. In addition to the indenting problem noted above, consider the following changes:

    # your code looks like an extract from a larger document # but just in case you aren't # do use strictures and declare your variables # it will save you *A LOT* of debugging time use strict; use warnings; # DECLARE YOUR VARIABLES! my $FH = \*STDOUT; my $target_db = $ARGV[0]; my $target_os = $ARGV[1]; my $flag; while (my $line = <DATA>) { if ($line =~ /WNT/ || $line =~ /UNX/) { print "$line\n"; } # be consistant: # if you are unsetting a flag with undef, then always use # defined to check for an unset flag. Not everything that # is false is also undef. if (defined($flag)) { # Fix some potential bugs/cause for warnings: # * check to see if variables are defined before using # equals or regex comparisons. Otherwise you will get # warnings. # Make code more readable and self documenting # * negations are harder to read than positives # * conditions shouldn't be buried inside an else {...} if # they apply to the entire content of the block. # * or'd regexes can be combined into one to make it clearer # that one is interested in a set of alternatives if (defined($target_db)) { if ($target_db eq 'sql') { next if ($line =~ /informix|oracle/i); } elsif ($line =~ /$target_db/i) { next; } } #* conditions that apply to the entire block should be # part of the if/elsif clause UNLESS you expect to be # expanding a simple condition to a full fledged if-else print $FH "$line"; if (defined($target_os)) { # leaving room for a more complicated target_os logic # later on if ($line =~ /$target_os/) { undef $flag; next; } } } #* defined is a fast check, so do it first #* defined($flag) is better than (defined $flag) #* double quoting a single variable is redundant. #* if we're not expecting more complicated logic # combine all conditions that apply to the block into # the if statement if (!defined($flag) && ($line =~ /UNX|WNT|COMPONENT/)) { $flag = 1; print $FH $line; } }

    Best, beth

Re: Looking for less code
by JavaFan (Canon) on Aug 11, 2009 at 09:34 UTC
    You can probably reduce the code more if you know more about the context, and the content of the file you are reading. There's an easy reduction of the line count if you use statement modifiers. Using $_ instead of $line reduces the amount of code as well:
    local $_; while (<DATA>) { print if /WNT|UNX/; if ($flag) { print if $target_db eq 'sql' && /informix|oracle/i || $target_ +db ne 'sql' && /$target_db/i; print $FH $_; undef $flag if /$target_os/; } elsif (/UNX|WNT|COMPONENT/) { $flag = 1; print $FH $_; } }
Re: Looking for less code
by Anonymous Monk on Aug 11, 2009 at 06:50 UTC
    Here you go Sun751, one line!
    while ($line = <DATA>) { if ($line =~ /WNT/ || $line =~ /U +NX/) { print "$line\n"; } if ($fl +ag) { if ($target_db ne 'sql') { + next if ($line =~ /$target_db/i); } + else { next if ($line =~ /informix/i | +| $line =~ /oracle/i) } print $FH "$line"; + if ($line =~ /$target_os/) { un +def $flag; next; } } if ( +$line =~ /UNX/ || $line =~ /WNT/ || $line =~ /COMPONENT/) { + if(!(defined $flag)) { $flag = 1 +; print $FH "$line"; } } }

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://787506]
Approved by moritz
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others avoiding work at the Monastery: (5)
As of 2024-03-28 15:00 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found