"This is my first Perl script."
++ This is an excellent, first effort!
"I'm looking for any kind of feedback, especially pitfalls and 'better' idioms etc."
Here's some observations; along with various suggestions and recommendations:
I'd personally avoid hard-coding a path to perl in the shebang line. Consider using the current environment's perl:
#!/usr/bin/env perl
This allows you to change the version of Perl in the current environment and run your script under this new version. In particular, it's useful when testing how your script runs under different versions of Perl (perlbrew is a common choice for this).
You're using the '/r' modifier in your regex substitution. The way you're using it is fine. The problem is, it was introduced in v5.14[perl5140delta: Non-destructive substitution]: anyone using an earlier version will have problems. The earliest version I have installed is v5.14.2, so I can't test this. Whenever I use the '/r' modifier, I typically start my scripts like this:
#!/usr/bin/env perl use 5.014; use strict; use warnings; ...
It's fine to use advanced features; however, if you do, it's a good idea to keep an eye on their statuses (e.g. stable, experimental, and so on). Take a look at the perl5delta and perl5vvsdelta[vv=version; s=subversion] manpages in the perldoc: perl: Miscellaneous section. New features are typically added in the first version (i.e. subversion=0): I typically start searches in the perl5vv0delta pages. Here's an example of the types of changes that can occur over just a few versions: v5.24 - works as expected; v5.22 and v5.20 - can be made to work with two features, warnings silenced and experimental status; v5.18 - fatal error. If '$x->@*' is uncharted territory, take a look at "perlref: Postfix Dereference Syntax".
$ alias perle alias perle='perl -Mstrict -Mwarnings -Mautodie=:all -E' $ perlbrew switch perl-5.24.0t $ perl -v | head -2 | tail -1 This is perl 5, version 24, subversion 0 (v5.24.0) built for darwin-th +read-multi-2level $ perle 'my $x = [qw{a b c}]; say "@{$x}"; say "$x->@*"' a b c a b c $ perlbrew switch perl-5.22.0t $ perl -v | head -2 | tail -1 This is perl 5, version 22, subversion 0 (v5.22.0) built for darwin-th +read-multi-2level $ perle 'my $x = [qw{a b c}]; say "@{$x}"; say "$x->@*"' a b c ARRAY(0x7ff4d2805480)->@* $ perle 'no warnings "experimental::postderef"; use feature qw{postder +ef postderef_qq}; my $x = [qw{a b c}]; say "@{$x}"; say "$x->@*"' a b c a b c $ perlbrew switch perl-5.20.0t $ perl -v | head -2 | tail -1 This is perl 5, version 20, subversion 0 (v5.20.0) built for darwin-th +read-multi-2level $ perle 'no warnings "experimental::postderef"; use feature qw{postder +ef postderef_qq}; my $x = [qw{a b c}]; say "@{$x}"; say "$x->@*"' a b c a b c $ perlbrew switch perl-5.18.0t $ perl -v | head -2 | tail -1 This is perl 5, version 18, subversion 0 (v5.18.0) built for darwin-th +read-multi-2level $ perle 'no warnings "experimental::postderef"; use feature qw{postder +ef postderef_qq}; my $x = [qw{a b c}]; say "@{$x}"; say "$x->@*"' Unknown warnings category 'experimental::postderef' at -e line 1. BEGIN failed--compilation aborted at -e line 1.
I don't see any reason for $VERSION; however, if you do need it, first see "version: SYNOPSIS". Of particular note:
... use version; our $VERSION = qv("v1.2.3"); # deprecated ... our $VERSION = "1.0203"; # recommended ...
Consider compiling your regex only once, and doing so at compile time. By the way, much of your escaping was unnecessary; I've removed the superfluous backslashes (i.e. all except '\\'); see "perlrecharclass: Special Characters Inside a Bracketed Character Class". I might have written something along the lines of:
... { my $re; BEGIN { $re = qr{(?x: [%/?<>\\:*|":] )} } for ... { ... s/$re/_/gr; } }
Take a look at "perlperf - Perl Performance and Optimization Techniques"; in particular, BENCHMARKS's "Search and replace or tr" section. While both substitution and transliteration can be used for this task, Benchmarking indicates transliteration can do it 25 times faster.
$ perle 'my $re; BEGIN { $re = qr{(?x: [%/?<>\\:*|":] )} } say q{%/?<> +\\:*|":} =~ s/$re/_/gr' ___________ $ perle 'say q{%/?<>\\:*|":} =~ y{%/?<>\\:*|":}{_}r' ___________ $ for i in {1..5}; do perle 'use Benchmark qw{cmpthese}; my $re; BEGIN + { $re = qr{(?x: [%/?<>\\:*\|":] )} }; my $str = q{%/?<>\\:*|":}; cmp +these 0 => { s => sub { $str =~ s/$re/_/gr }, y => sub { $str =~ y{%/ +?<>\\:*|":}{_}r } }'; done Rate s y s 212144/s -- -96% y 5420361/s 2455% -- Rate s y s 204213/s -- -96% y 5632150/s 2658% -- Rate s y s 201600/s -- -97% y 5771041/s 2763% -- Rate s y s 204103/s -- -96% y 5026909/s 2363% -- Rate s y s 222651/s -- -96% y 5600560/s 2415% --
Perhaps a regex substitution was not the best choice.
I don't see any need for the intermediary variable '$uri': you can just write 'push @uris, shift'. I'd also question the need to create a new coderef on every iteration. Consider something like one of these two ways (untested):
{ my @uris; my $finder_code = sub { push @uris, shift }; sub finder_sub { push @uris, shift } for ... { ... @uris = (); my $code_finder = URI::Find::->new($finder_code); my $sub_finder = URI::Find::->new(\&finder_sub); ... } }
If you were wondering about the extra pair of colons I added ('::->'), see "perlobj: Invoking Class Methods".
There may be other issues, but these were the ones that stood out for me.
Having pointed out all those issues, I just wanted to reiterate that this is a well-written script and an excellent first attempt.
See also: the online perl manpage (which I have bookmarked and refer to often); "Create A New User".
Update: Fixed typo: s/obversations/observations/
— Ken
In reply to Re: Please critique this script -- Read emails and write URLs into bookmarks
by kcott
in thread Please critique this script -- Read emails and write URLs into bookmarks
by Anonymous Monk
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |