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. | [reply] [Watch: Dir/Any] [d/l] [select] |
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. | [reply] [Watch: Dir/Any] [d/l] [select] |
|
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.
| [reply] [Watch: Dir/Any] |
|
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
| [reply] [Watch: Dir/Any] [d/l] [select] |
|
|
|
|
|
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.
| [reply] [Watch: Dir/Any] [d/l] [select] |
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 | [reply] [Watch: Dir/Any] [d/l] |
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 $_;
}
}
| [reply] [Watch: Dir/Any] [d/l] |
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"; } } }
| [reply] [Watch: Dir/Any] [d/l] |