G'day oldB51,

Don't write "My code appears to work." and leave us to verify it; show your output. It didn't work for me because:

$ perl -e 'print “\n”;'
Unrecognized character \xE2; marked by <-- HERE after print <-- HERE n
+ear column 7 at -e line 1.
[download]

Using a subroutine is usually a good thing. It abstracts your code so it can be reused within your script. If you find it's particularly useful, you can put it in a module and many scripts can reuse it.

There are many functions whose default argument is $_. Writing abs instead of abs($_) is absolutely fine. Pun intended. :-)

Aim for readability and maintainability. The following might look clever but, for many, it would be difficult to understand; it's not easy to modify; and, it can't be reused.

#!/usr/bin/env perl

use strict;
use warnings;

use List::Util 'sum';

my @input = qw{-222 -221 -21 0 1 1 2 3 5 8 13 21 34 55 89 144};

print join(' ', map +(sum(split //, abs) % 2 ? $_ : ()), @input), "\n"
+;
[download]

Output:

-221 -21 1 1 3 5 21 34 89 144
[download]

Always use the strict and warnings pragmata. You may get one or both of those for free, with different values of VERSION, if you "use VERSION;". Some modules also do this for you.

I would also recommend that you unpack @_ at the start of your subroutines. There are other variables that you should deal with as soon as they become available; for example, $@ containing errors; $1, $2, and so on from regex matches; etc. There are various things that can change their values, perhaps not in the code when first written, but via insidious bugs that might be introduced following modifications. The following would've been better:

sub oddDigitSum {
    my (@nums) = @_;
    my @ans;
    for (@nums) {
    ...
[download]

Of course, all of the above are my opinions. Others may have different opinions. Form you own opinions as your experience grows. :-)

Thanks for a very helpful reply. I did have

 
#!/Users/barry/localperl/bin/perl
use 5.036;
use warnings FATAL => 'all';
[download]

at the head of my code but forgot to include it in post. I will also include output next time. Sorry, first post but will remember in future.

"Thanks for a very helpful reply."

You're very welcome. I'm always happy to help those who are eager to learn.

"I did have ... forgot to include it ... Sorry, first post but will remember in future."

That's fine and no need to apologise. My first post, a dozen years ago, was a complete mess: it took me several edits to clean it up.

When I don't know what version someone is using, I'll usually default to code that works in v5.8. Had I known that you were using the current latest, v5.36, I would've made a few more suggestions.

A simple one is say(), which has been around since v5.10. That saves you having to tag "\n" onto the end of print statements: not always what you want, but often is.

Of more recent vintage is "subroutine signatures". That's been flagged as experimental for quite a few versions, but no longer in v5.36. My suggested change for oddDigitSum() could have been written as:

sub oddDigitSum (@nums) {
    my @ans;
    for (@nums) {
    ...
[download]

When writing code just for myself, I typically use the latest version. If others are involved, I sometimes need to wind that back (or at least advise what newer features I'm using that require that version).

The online documentation, https://perldoc.perl.org/perl, shows the latest version; it does provide easy access to earlier versions. I'd suggest using that while you're learning; although, I would recommend staying away from experimental features: do read about them; don't use them in code, as many may change or even be removed; wait until they become stable.

— Ken

As an example, I have used the absolute value function abs but I could have used abs($_). Which is best? Does it matter?

Specifically on this point: others have pointed out that there is no difference functionally, so that leaves style and clarity to drive the choice.

In my case, I hardly ever use the abs function (despite the fact that most of the code I write is maths-related). Because of that, I would choose always to write $abs = abs($_); rather than $abs = abs; - for the latter, I'd have to stop and think a few deciseconds longer any time I wrote it or read it.

On the other hand, for functions that default to $_ that I use commonly, I'm much more likely to prefer the short form.

On the gripping hand, I try to avoid over-reliance on $_. Normal for loops are almost always improved by naming the variable. I mostly have $_ in "right-to-left" code - maps, greps, modifier-for - and there, again, I look to name a variable if I'm going to be using it several times:

# different $_ on left and right, but I find this perfectly clear
$sum += $_ for /(\d)/g;
# or
my @saints = grep $_->is_saint, @users;
# or
my @users = map {
  my $name = $_;
  is_god($name)
    ? find_with_sacrifice($name, $spare_user)
  : is_saint($name)
    ? find_with_relic($name, $relic)
  : $spare_user
    ? find_user($name)
  : do {
    # save for later
    $spare_user = find_user($name);
    ()
  }
} @names;
[download]

Yet another inevitable TMTOWTDI:

#!/usr/bin/env perl

use strict;
use warnings;
use feature qw(say);
use Math::Prime::Util qw(sumdigits);
use Data::Dump;

my @input = (-222, -221, -21, 0, 1, 1, 2, 3, 
              5, 8, 13, 21, 34, 55, 89, 144);

my @ans = map { (sumdigits($_) % 2) ? $_ : () } @input;

dd \@ans;

__END__

Karls-Mac-mini:monks karl$ ./11146306.pl
[-221, -21, 1, 1, 3, 5, 21, 34, 89, 144]
[download]

See also map, Data::Dump, Math::Prime::Util and ntheory.

Best regards, Karl

> Yet another inevitable TMTOWTDI:

KISS! s/map/grep/ ;-)

my @ans = map { (sumdigits($_) % 2) ? $_ : () } @input;

...
my @ans = grep { sumdigits($_) % 2 } @input;
...
[download]

Cheers Rolf
_{(addicted to the Perl Programming Language :)
Wikisyntax for the Monastery}

Or even:
    my @ans = grep 1 & sumdigits($_), @input;

---

Give a man a fish:  <%-{-{-{-<

"Monachus quidem varietas maxime delectat". Or so.

«The Crux of the Biscuit is the Apostrophe»

These are incredible modules Karl. Clearly it is important to know what pre built modules are available. That said, at this stage of my Perl journey, I'm glad I built my programme without the modules first. Thanks for you help.

Yes, sure. Some more inspiration might be found here. Sometimes it is good to take a look under a hood.

Read more... (870 Bytes)

Regards, Karl

«The Crux of the Biscuit is the Apostrophe»

1. Don't push when you can print.
2. Usually better to make sub work on one number rather than an array of numbers.
3. Always use strict and use warnings.
4. You don't have to predeclare the sub -> Ok to put the loop on the input first.
5. If I have a good name for the sub, I may not have to be too interested in how it works. With this ordering, I see right away what the program is supposed to do.

use strict;
use warnings;

my @input = ( -222, -221, -21, 0, 1, 1, 2, 3, 5, 8, 13, 21, 34, 55, 89
+, 144 );

foreach my $int (@input)
{
   print "$int " if isSumDigitsOdd($int);
}
print "\n";

sub isSumDigitsOdd
{
    my $num = shift;
    my $sum;
    $sum += $_ foreach (split //, abs $num);
    return ( $sum % 2 );  # return ( $sum & 1 ); would be fine also
}

#prints: -221 -21 1 1 3 5 21 34 89 144
[download]

Also:
As an example, I have used the absolute value function abs but I could have used abs($_). Which is best? Does it matter?

I would write abs($_) or even abs $_ rather than just "abs" just to make things more clear even though the generated code is the same. Often there is some descriptive name other than just $_ and I use $_ for simple one line loops like above. Naming loop iterator variables is cheap and improves readability - do it often!

Don't push when you can print.

I'm sorry, this is not good advice. It creates a "strong coupling" between the logic of the algorithm and the interface/caller. It may be that you decide later that you want to send the results somewhere else, or do further processing on them before outputting. Do you really want to dive into the algorithm and find all the print statements? You'll just be converting them to push'es anyway. Better advice is to have your algorithm build its results in a data structure; have a separate routine for outputting that data.

Thank you for this jdporter.

Your practical comments are extremely helpful.

Not sure what the 'looks like homework' jibe was meant to achieve. I'm 71, have been retired for nearly 14 years and I don't do homework in the sense that term is usually understood. I have been learning Perl for 6 days.

Nonetheless your other comments are extremely useful and I thank you for them.

Oh, sometimes we get a student who are lazy and trying to trick us into doing their homework for them. But usually those folks don't show up with anything close to working code!

After your explanation, I am impressed! Your code demonstrates a level of understanding that extremely few 6-day beginners possess. And your question wasn't "how do I do it?", it was "how do I do it better?".

Keep going! You are off to a very fine start!

One path to learning is to take a problem one step further (and then repeat, etc.).

In your OPed code and (I think) in all the replies, the sign of the number being tested is ignored through the use of abs.

What happens if you do not ignore the sign of the number, so that, e.g., -321 becomes the list (-3, 2, 1) with the sign applying to only the first digit? Is any interesting property of the problem or of numbers in general revealed?

Remember that Perl will seamlessly convert numbers back and forth between numeric and string representations. If -321 is represented as '-321', might regular expressions be used to extract substrings that might then be treated as numbers? (Please see perlre, perlretut, and perlreref/perlrequick.)

Thank you AnomalousMonk. I think the point you are making is that when summing digits: if we consider integers of the form abc say then -abc will always sum to 2a less than abc. 2a is always even so abc and -abc must always both be odd or both even. Hence no need to worry about absolute values for this purpose. Please forgive me if I have not expressed this in a correct formal way - it is over 40 years since I did any mathematics. Your point is excellent.

... no need to worry about absolute values for this purpose.

Yes, that's the point I was getting at. I also wanted to hint that you don't even need to be concerned with the numbers themselves, but only with the least-significant bit of each number (in 2s'-complement binary representation). The LS bit determines the odd/even status of a number, so adding up all these bits mod 2 (i.e., masking with 1) gives the answer.

Please forgive me if I have not expressed this in a correct formal way ...

No worries. I doubt if I would recognize a correct formal expression of this proposition if you hit me in the face with it.

---

Give a man a fish:  <%-{-{-{-<

My 2 cents CDN, for what it's worth. (We don't have pennies anymore, so 0.)

$ perl -e'
my @input = ( -222, -221, -21, 0, 1, 1, 2, 3, 5, 8, 13, 21, 34, 55, 89
+, 144 );
sub oddDigitSum {
    my @ans;
    for ( @_ ) {
        my $sum;
        $sum += $_ for /\d/g;
        $sum % 2 && push @ans, $_;
        }
    return @ans;
    }
print "@{[ oddDigitSum @input ]}\n";
'
[download]

Thanks for this jwkrahn. I had already amended my code to use /\d/g. There are so many different ways of attacking a problem.

my @digits = split //, abs;
with:
my @digits = split //, $_;
[download]

C:\_32\pscrpt>perl try.pl
Argument "-" isn't numeric in addition (+) at try.pl line 10.
Argument "-" isn't numeric in addition (+) at try.pl line 10.
Argument "-" isn't numeric in addition (+) at try.pl line 10.
-221 -21 1 1 3 5 21 34 89 144
[download]

no warnings 'numeric';
[download]

sub oddDigitSum {
    no warnings 'numeric';
    my @ans;
    for(@_) {
        my @digits = split //, $_;
        my $sum;
        $sum += $_ for @digits;
        $sum % 2 && push @ans, $_;
    }
    return @ans;
}
[download]

I would strongly favor using the abs() function. One of the issues here is that if you have a statement like no warnings 'numeric';, then there should be some comments explaining why that is. abs() requires no explanation.

As far as speed with Perl, I am betting that abs() is really fast - this is not a complex operation for a 2's complement number. Don't know that Perl would be "smart" enough to do this asm code, but this is very fast because no branches to stall the instruction pipe.

; abs(eax), with no branches.
; intel syntax (dest, src)

mov ebx, eax   ; save copy of eax
neg eax
cmovl eax, ebx ; if eax is now negative, restore its saved value
[download]

Um, I hate to break it to you, but perl is fully interpreted with no just-in-time compilation. Every operation and every argument passed to it in a script is dozens of branches, no matter how innocent. The way to optimize a perl program is to code it with the fewest possible operations, fewest function calls, fewest assignments to a temporary variable, and even fewest curly-brace scopes. Optimizing a perl program is generally the opposite of making it more readable. (but there is a time and a place, etc)

For a little mind-bender, try this:

perl -MBenchmark -E '
  my $x= 1;
  timethese(50000000, {
    mul => q{$x*$x},
    sqrt => q{sqrt($x)}
  })'
[download]

Thanks for your help.

Talk of optimization is premature?

Fantastic re no warning numeric. Works fine without the abs.

If you don't want to use abs and you don't want the warnings, there's always

my @digits = /\d/aag;
[download]

---

🦛

It is, in my humble opinion, best practice to collect up the incoming parameters of a subroutine as soon as humanly possible so while reading into the function/method, it's easy to see what is what (the length of the variable name isn't my normal style, I've just made it readable for the example):

sub oddDigitSum {
    my @signed_integers_to_work_on = @_;

    my @ans;

    for (@signed_integers_to_work_on) {
        ...
[download]

To further, I always check my input parameters, even if a built-in will throw an exception on them anyway (the regex isn't always my normal way of checking, but it's good as an illustrative example):

sub oddDigitSum {
    my @supplied_signed_integers = @_;

    my @ans;

    for my $signed_int (@supplied_signed_integers) {
        if ($signed_int !~ /^-?\d+$/) {
            die "Parameter '$signed_int' isn't a signed integer";
            ...
[download]

For me, I use the implicit $_ variable when my loop is exceedingly short in lines of code, or the complexity of the loop is only one level deep and very clear to understand. Your for() loop is a good example of where I'd use $_ and not bother declaring and defining a named scoped variable for each value being iterated as I did in an above example. If there was even one more operation in that for() loop though, I'd break it up with named variables, just so its easier to a future reader (often me) which operation modified the list.

Thanks for this stevieb. I have amended my code so as to check input parameters. For the moment I have done it by returning a nice friendly message rather than using  die. However, I will be returning to this when I can study errors properly.

hi

Party of the first part? Smart quotes snuck in your post;)

compare to yours, what did i change?

use strict;
use warnings;
my @input = ( -222, -221, -21, 0, 1,
 1, 2, 3, 5, 8, 13, 21, 
34, 55, 89, 144 );
print $_, " " for oddDigitSum(@input);
print "\n";
exit 0;

sub oddDigitSum {
    my @ans;
    for my $int (@_) {
        my @digits = split //, abs $int;
        my $sum = 0;
        $sum += $_ for @digits;
        $sum % 2 and push @ans, $int;
    }
    return @ans;
}
[download]

compare to yours, and mine, what did i change?

use strict;
use warnings;
my @input = ( -222, -221, -21, 0, 1,
 1, 2, 3, 5, 8, 13, 21, 
34, 55, 89, 144 );
print $_, " " for oddDigitSum(@input);
print "\n";
exit 0;

sub oddDigitSum {
    my @ans;
    for my $int (@_) {
        my $sum = 0;
        $sum += $_ for split //, abs $int;;
        $sum % 2 and push @ans, $int;
    }
    return @ans;
}
[download]

compare to yours, and mine, and mine, what did i change? Is it better? Simpler? Clearer? New and unfamiliar? Ambiguous? Faster?

use strict;
use warnings;
my @input = ( -222, -221, -21, 0, 1,
 1, 2, 3, 5, 8, 13, 21, 
34, 55, 89, 144 );
print $_, " " for oddDigitSum(@input);
print "\n";
exit 0;

sub oddDigitSum {
    my @ans;
    for my $int (@_) {
        my $sum = Summ(
split //, abs $int );;
        $sum % 2 and push @ans, $int;
    }
     @ans;
}
sub Summ {
  my $sum = 0;
  $sum+=$_ for @_;
  $sum;
}
[download]

I'm typing this on my phone. Seent map yet? I've seent it :) is it ...?

use strict;
use warnings;
my @input = ( -222, -221, -21, 0, 1,
 1, 2, 3, 5, 8, 13, 21, 
34, 55, 89, 144 );
print $_, " " for oddDigitSum(@input);
print "\n";
exit 0;

sub oddDigitSum {
    my @ans = map {
        my $sum = Summ(
split //, abs $_ );;
        $sum % 2 
        ? $_ # keep oddities
        : () ;
    } @_;
     @ans;
}
sub Summ {
  my $sum = 0;
  $sum+=$_ for @_;
  $sum;
}
[download]

I'm typing this on my phone. Seent map yet? I've seent it :) is it ...?

use strict;
use warnings;
my @input = ( -222, -221, -21, 0, 1,
 1, 2, 3, 5, 8, 13, 21, 
34, 55, 89, 144 );
print $_, " " for oddDigitSum(@input);
print "\n";
exit 0;

sub oddDigitSum {
    map {
         my $sum = 0;
         $sum+=$_
            for split //, abs $_ ;;
        $sum % 2 
        ? $_ # keep oddities
        : () ;
    } @_;
}
[download]

im sleepy

there is discussion of things like this, but I'm away from my keyboard and my links, so these search terms are a reminder to me to follow up on this question

on Naming variables use $_....define best, what best, readability, idiomatic, modern Perl, local $_, reuse var