https://issues.apache.org/SpamAssassin/show_bug.cgi?id=6312

--- Comment #4 from Mark Martinec <[email protected]> 2010-01-29 16:37:13 
UTC ---
> I read that backwards at first glance, having missed the "() :" portion and
> then wondering why this adds two potentially missing numbers.  I'm not as
> firmly planted a perl developer as many others, but why not use the less
> confusing negation,
> +  my $mail = $spamtest->parse(\...@msglines, 0, $timeout_child && 
> $start_time ?
> +                { master_deadline => $start_time + $timeout_child } : () );

It is partly a matter of preference, but my general principle when choosing
order of my   'if elsif elsif else'   sections is to deal with trivial and
short cases first, and leave the more bulky code for later. Same principle
as writing recursive code. This keeps requirements for my (code reader's)
mental stack low: reading it sequentially, understand and quickly digest
short sections, forget about them (push off the mental stack), than give
all attention to the bulky part.

In a more realistic example, that would be:

* my preference:
  if (!open(...)) {
    report error
  } else {
    do all
    the
    bulky
    processing
  }

instead of:
  if (open(...)) {
    do all
    the
    bulky
    processing
    ...one screenfull below...
  } else {
    report error
  }

There may be exceptions to the rule, like optimizations, conditional
expressions overlap, etc, but other things being equal, code size decides
my order.

While our one-liner sample may not be the best example, I applied the
same principle there - but I can live with it being reversed, if that is a
general feeling.


> ... ideally without the ternary operator,
> 
> +  my $mail = $spamtest->parse(\...@msglines, 0, $timeout_child && 
> $start_time &&
> +                       { master_deadline => $start_time + $timeout_child } );

This is different and can be wrong. My code provides one argument less
when condition is false (i.e. no optional arguments hash), yours
adds an undef as the last argument if the condition is not met.
Now, depending on what a subroutine expects this may be acceptable too,
but without double checking the called subroutine I prefer to stay
on the safe side.


> ... or since you'll probably object to that short-circuit nature,
> 
> +  my $mail = $spamtest->parse(\...@msglines, 0, 
> +                       if ($timeout_child && $start_time)
> +                       { master_deadline => $start_time + $timeout_child } );

This is syntactically incorrect, a statement can not stand within an
expression. It would need to be enclosed in a do { ... }, which would be
unsightly, and suffer the same problem as the case #2.

-- 
Configure bugmail: 
https://issues.apache.org/SpamAssassin/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

Reply via email to