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]

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

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.

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]

++ 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

"Your distribution doesn't appear to be in a VCS"

It seems to be on github. bod see META_MERGE.

It has been uploaded to GitHub...but I haven't done much with it there. Any development is still being done with local copies whilst I slowly learn how to properly use GitHub...

The META files are something I need to get an understanding of. It's on my list of things to investigate and no doubt there will be a question to The Monestry at some point...

It's a case of TIMTOTWDI, and I have not looked at the original code, but two comments:

No need for this: $self->{'error'} = '';. It'll be autovivified if needed.

It will autovivify as undef if it is not assigned to before it is accessed. That could cause issues later on with other code.

There's no need for a return; at the end of a sub if nothing's being returned (unless you intentionally are returning undef.

It's safer to explicitly return as otherwise the value of the last expression is returned. FWIW, this is covered in PBP on p197 under Implicit Returns, with a follow-on under Returning Failure on p199 (although that also encourages the Contextual::Return module which I don't use).

It's safer to explicitly return as otherwise the value of the last expression is returned.

He did say "unless you intentionally are returning undef". I agree with stevieb on that point; however, in my code if I am intentionally returning undef I make that explicit with return(); I never use return; in my code unless I am returning early from a sub which is not expected to return a value. (If the caller insists on doing something with the returned value from such a sub, TNMP.)

Thanks for the clarification. Two other points, which you are probably well aware of but some readers might not be:

return; and return(); are the same, although the brackets do make it more visually distinct and perhaps that's your point.

return; in list context returns the empty list. This means it is effectively the same as return wantarray ? () : undef; unless it is called in void context. Your use of it when not expecting a return value implies void context, though, so perhaps this point is moot.

Lots of good advice in there. I'm not averse to a bunch of returns inside a sub especially if it simplifies the code and am also quite happy with my $x = shift; so long as there is only one arg being retrieved. As time goes on I am becoming less tolerant of unnecessary punctuation as I have to pause and question why it is there so that gets a big thumbs-up from me.

There's no need to eval $VERSION: $VERSION = eval $VERSION;

This was covered a few weeks ago where the original source for it was unearthed. Still not convinced of the need for massaging the version like this but if I were I would go with Haarg's approach instead - for clarity if nothing else.

Brief reply as I'm about to go to bed but I'll reply to this one now and look properly at the rest over the next few days...

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

It is quite possible that $vars{'payload'} could contain badly formed JSON. If it does, decode_json calls die (or something equally terminal). I want to catch that so that my module doesn't exit. Then the user of the module can handle the error as they need. If the module were to die, the return to Stripe would be invalid or, more likely, non-existent which may cause issues with future webhook calls.

You can interpolate hash values. Instead of my $signed_payload = $sig_head{'t'} . '.' . $self->{'payload'};, you can simply do: my $signed_payload = "$sig_head{t}.$self->{payload}";

I'm aware that I can but that doesn't mean I should.
To my mind, referenced variables are clearer outside interpolation

There's no need for a return; at the end of a sub if nothing's being returned (unless you intentionally are returning under;

I am intentially returning under. The value is returned earlier in the method.
It is possible that a rogue return may have been left in. However, I've been bitten in the past with a sub returning an unexpected value because there was no explicit return statement. So I now nearly always include one...

However, hopefully you find some use in my quick code review here.

Yes, very useful. Thank you for taking the time to review the code.

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}

The reasons I single quote hash keys are:

1. it makes them obvious as literals because a text editor highlights them.
2. it differentiates them from interpolated hash keys - e.g. $vars{"key_$count"};
3. it is easier to expand into variable keys

I think of 3 in the same way I been told that:

if ($condition) {
    $var = 1;
}
[download]

$var = 1 if $condition;
[download]

Likewise, I feel:

$vars{'example'} = 1;
[download]

$vars{example} = 1;
[download]

$vars{'example' . $count} = 1;
[download]

Others may have different opinions, clearly they do as there is a lot of code with unquoted hash keys, but this works for me in terms of clarity, expandability and maintainability. I don't see any downside to is on performance (please correct me if there is!) so I think I shall keep this one the way it is.

Better is in the eye of the beholder and depends on too many preferences to define properly.

e.g. run () if $chased is exactly the same code as $chased and run ();. It fully depends on your brain, the size of the team (1 is a valid size) and the style/rules the team has agreed on, the code consistency and the context which of the two to prefer. Personally I hate statement modifiers, as it does not fit my brain, so I prefer "expression and action" over "action if expression", but it it neither better nor worse.

With your reasoning, *I* always prevent single quotes wherever possible, so that /when/ I see them, they are special. I made an exception in a CPAN module that I want to be as portable as possible, even under the most strict release versions of perl, even some experimental ones.

I personally find $vars{"example $count"} way more readable and intuitive than $vars{'example ' . $count}

Your milage may/will vary. Be consistent, which is way more important than being conventional.


Enjoy, Have FUN! H.Merijn

With your reasoning, *I* always prevent single quotes wherever possible, so that /when/ I see them, they are special.

Precisely this. Conversely, if the code contains extra syntax over what is necessary then the reviewer (who might also be the author) needs to spend extra time considering why the extra syntax is there. Less syntax = less to parse = less to get wrong or be confused over. Less is more. :-)

---

🦛

In my experience, there are two ways of using hashes. One is as a lazy-programmer's object. You see these kinds of hashes returned from database queries, received from REST API requests, etc. Something like this:

my $uncle = {
  name => 'Bob',
  age  => 45,
};
[download]

In these cases, the keys are an already-known set of strings, usually picked to be identifier-like (no spaces, weird punctuation, etc), so I will usually not quote keys in this kind of hash.

The other way of using hashes is as basically a mapping from values of one type to values of another type. For example:

my %ages = (
  'Alice'    => 41,
  'Bob'      => 45,
  'Carol'    => 38,
);
[download]

Quoting the keys for this kind of hash is useful because you never know when another value is going to be added which isn't a safe identifier ($ages{"d'Artagnan"} = 62) plus it helps visually distinguish the second kind of hash from the first kind of hash.

Do I consistently follow this rule? Absolutely not.

Hire me at Toby Ink Ltd or Join my OnlyFans