in reply to Code review

This is some weird looking code and appears to contain at least one error. To have a clear, concise algorithm, I would go up one level in problem statement and start with the intent of the program in general words.

my %Y;
while (<>) {
  ...
  my ($y) = ... ;
  next unless $y;
  
  # Add the scalar number of elements in @ARGV to the hash
  # Always add at least 1 to the hash even if @ARGV contains
  # no elements
  # the value $y is meaningless and plays no role
  
  if(@ARGV) {
      # next is always executed regardless of $y eq $_
      foreach (@ARGV) { $Y{$y}++,next if $y eq $_;  }
  } else { $Y{$y}++ }
}

[download]
So, this single statement is possible:

scalar @ARGV ? $Y{$y} += @ARGV : $Y{$y}++;

[download]
1. The comma operator serves no purpose in the above code. Prefer ; instead.
2. If you want the test to short circuit, then it should go first, not last!:
foreach (@ARGV) { next if $y eq $_; # don't count any occurrences of
                                    # $y
                  $Y{$y}++,
                }

[download]
Or, this is a possibly of what was meant:

scalar @ARGV ? $Y{$y} += (grep $y ne $_, @ARGV) : $Y{$y}++; Without some more English language explanation, I have no idea what the code is supposed to do.

Replies are listed 'Best First'.
Re^2: Code review
by hv (Prior) on Mar 07, 2024 at 12:14 UTC

    I would characterize the effect of the code as:

    For each input line, process it and extract an id. When run with no arguments, count in %Y the number of times each id appears; when run with a list of ids as arguments, count only how often the specified ids appear (with multiplicity).

    I suspect you have been misled by a misinterpretation of:

    foreach (@ARGV) { ++$Y{$y}, next if $y eq $_; }

    [download]

    which is equivalent to:

    foreach (@ARGV) {
    if ($y eq $_) {
        ++$Y{$y};
        next;
    }
}

    [download]

    (I assume the useless next is left over from cutting down the original code.)

    This is an idiom I actually use quite commonly for compactness when parsing arguments:

    while (@ARGV && $ARGV[0] =~ /^-/) {
    my $arg = shift @ARGV;
    last if $arg eq '--';
    ($max = $arg || shift(@ARGV)), next if $arg =~ s{^-x}{};
    $inject = 1, next if $arg eq '-i';
    ++$debug, next if $arg eq '-d';
    die "Unknown option '$arg'\n";
}

    [download]
[reply]
[d/l]
[select]

In Section Seekers of Perl Wisdom