Re^2: What to test in a new module

Instead of my $class = shift;, you should be doing my ($class) = @_;
my $self = shift; my %keys = @_; is better written as my ($self, %keys) = @_;

I disagree. The object (or class name, in the case of static calls) is qualitatively different from the rest of arguments. I also think it's only fair to point out that, except in the one case of %keys = @_ you cited, he very consistently used the shift style only when there were no further arguments, and otherwise used the style you advocate, e.g. my ($self, $subscription, $secret) = @_;

Reduce a ton of noise by eliminating quotes in hash keys when they don't contain anything other than alphanum and underscore. This: $vars{'error'} becomes this: $vars{error}

I disagree. quotes aren't noise. Worse, in my opinion, is the inconsistency in having some keys with quotes and others without which arises when, eventually, you have some keys which must be quoted.

Why are you doing an eval on this line... you do nothing at all if eval fails: $vars{'webhook'} = eval { decode_json($vars{'payload'});};

What's the mystery? Did you miss the following line? Clearly, decode_json could throw, but he doesn't care about that, he just wants an undef in that case. And he checks for it.

Instead of a bunch of return undef; inside of many if/else statements, create a flag at the top of the sub, and have each if/else set it. Return at the end of the sub.

I disagree. He is usually doing this when an error has occurred and there's no point in proceeding. Essentially, this is him using a magic / out-of-band return value (undef) instead of throwing an exception. It is an entirely valid way of doing things. (Of course, there is much debate out there on whether it's better than exceptions.) More importantly, it makes the code much, much simpler than a multiway/nested if-else construction.

$self->{'webhook'}->{'type'}; can be written as $self->{webhook}{type};. Again, reduces noise.

It's a matter of style only; and some people may find it nicer. Again: not noise. You seem to think that any characters which could possibly be eliminated are necessarily "noise".

You can interpolate hash values

You can... but concatenating is faster.

nor can I sort out at a glance why you're using the & for certain calls

Indeed,

&{$self->{$hook_type}}($self->{'webhook'});
[download]

is better written as

$self->{$hook_type}}->($self->{'webhook'});
[download]

... and IMHO this is one reason why I might be tempted to use deref arrows at other points in the reference chain besides the first: for consistency of style. (I don't, though. Not usually. When I do, it's for other reasons. :-)

Comment on Re^2: What to test in a new module Select or Download Code

Replies are listed 'Best First'.
Re^3: What to test in a new module by Bod (Parson) on Jul 08, 2023 at 17:08 UTC
You can... but concatenating is faster I hadn't thought of the speed implications. But I guess concatenation is always going to be faster than interpolation.	[reply]
Re^4: What to test in a new module by Arunbear (Prior) on Jul 09, 2023 at 11:16 UTC
I usually prefer interpolation because I can see at a glance what the result will look like, so there is less mental work to do when reading it. sprintf is a good middle ground between interpolation and concatenation. I sometimes use that when the variable names are long enough to interfere with the readability of interpolation. `my $signed_payload = $sig_head{'t'} . '.' . $self->{'payload'}; my $signed_payload = sprintf '%s.%s', $sig_head{'t'}, $self->{'payload +'};` [download]	[reply] [d/l]
Re^5: What to test in a new module by jdporter (Paladin) on Jul 10, 2023 at 14:22 UTC
Also: `my $signed_payload = join '.', $sig_head{'t'}, $self->{'payload'};` [download]	[reply] [d/l]
Re^4: What to test in a new module by kcott (Archbishop) on Jul 09, 2023 at 10:51 UTC
++ My response may come across as negative. That's not my intention so I'm just letting you know that I upvoted your post before replying. :-) "I hadn't thought of the speed implications." Unless you're experiencing noticeable speed issues, continue to not think about it. This sort of micro-optimisation is rarely useful: you'll spend inordinate amounts of time possibly saving just some nanoseconds when it would be more productive to focus on the code in general. "But I guess concatenation is always going to be faster than interpolation." Don't guess; instead, Benchmark. I seem to recall 20+ years ago, when coding to `v5.6`, various forms of concatenation (e.g. `join '', $x, $y` or `$x . $y` or `"$x$y"` or others) provided certain performance benefits. Reading the optimisation sections of release notes over the years has shown that these benefits have largely dissipated. In the main, I would focus on readability. For instance, many authors eschew whitespace, resulting in code like `$x.'.'.$y` which I find difficult to read at first glance, and often require me to do a doubletake to determine what each dot is doing. This advice pertains to many aspects of Perl syntax, not just concatenation, which perhaps had some benefits in the past but have been optimised away as the language has matured. — Ken	[reply] [d/l] [select]
Re^5: What to test in a new module by Bod (Parson) on Jul 09, 2023 at 22:43 UTC
Unless you're experiencing noticeable speed issues, continue to not think about it (speed) Don't guess; instead, Benchmark I was guessing because I wasn't dwelling on speed :) Unless I have to process lots of something with a big loop, I don't tend to think about speed unless it becomes an issue - my guess was about academic interest rather than practical concern. Thanks for the benchmarking link - I have intended to try benchmarking something just so I know how to do it.	[reply]