Beefy Boxes and Bandwidth Generously Provided by pair Networks
Just another Perl shrine
 
PerlMonks  

Re: Error Reporting from Module

by rjt (Curate)
on Oct 09, 2019 at 09:59 UTC ( #11107233=note: print w/replies, xml ) Need Help??


in reply to Error Reporting from Module

Congratulations on your first module!

I've taken a look through your documentation and code, in that order. The main issue I have with the error reporting that I have is it's somewhat inconsistent. You croak() in a bunch of places (e.g., for invalid parameters), but return undef in others (with a warning), so the programmer has to check for both. And warnings/carp() have their place, but can be annoying to deal with if they are "expected" in normal operation. Overriding $SIG{__WARN__} is technically workable, but obviously not ideal, especially when they also have to deal with the other error modes.

In your case, I would suggest picking one option and sticking with it, unless you have very good reason, and even better documentation. The exception model isn't always the best, but seems reasonable in your case. When using exceptions, use carp() sparingly, and only for things that require a warning, but do not indicate a significant deviation from normal program flow, in my opinion. (carp() followed by an early return is usually a bad sign.) (Minor edit for clarity)

This is just one tired old dev's opinion, but you usually can't go too far wrong with exceptions for a module like yours. It gives the caller the option to easily pass through (and thus exit on error), catch and handle, catch and ignore, and all of that is easy to do with basic Perl code, or even add a little syntactic sugar and extra features like Try::Tiny if desired.

Now, I have a few other comments for your code, in the readmore, if you like unsolicited advice. :-)

Loops: You've got C-style for ($i = 0; $i < foo(); $i++) loops all over the place. The Perlish way to do these is for (0..foo()) for numbers, and most importantly, for (@list) for lists (you had for ( my $i = 0; $i < scalar @{ $params{files} }; $i++ ) { ... } on line 326, for example.for (@{$params{files}}) { ... } is the much more readable alternative.)

Scalar context: You check for array length with if (scalar @_ < 1). Nothing wrong with that, but the scalar is unnecessary in this case. Fine to leave it in if you feel it clarifies your intent, but I usually leave it out.

You have the following code:

my %params; if ( $params{content} ) { $request{content} = $params{content} } if ( $params{username} ) { $request{username} = $params{username +} } if ( $params{avatar_url} ) { $request{avatar_url} = $params{avatar_u +rl} } if ( $params{tts} ) { $request{tts} = JSON::PP::true }

I'd probably write it more compactly (and more extensibly) like so:

my @valid_keys = qw< content username avatar_url >; my %request = map { $_ => $params{$_} } grep { $params{$_} } @valid_ke +ys; $request{tts} = $params{tts} ? JSON::PP::true : JSON::PP::false;

A more general note to learn and fully consider the differences between boolean truth, exists, and defined will help you avoid some pain. I have not reviewed your code or the Discord API in enough detail to know which you should be using in this case.

Some of your subroutines are quite long and contain code that is close enough to being repeated. You could gain quite a bit by some refactoring. Pull out parts, make them more generic, and use them in multiple subs. It'll be more maintainable and easier to understand. As a very rough rule of thumb, I rarely go beyond 20-30 lines for a sub without good reason. Some of yours are about 100 lines. You do use more vertical whitespace, so that number is a bit high, but then again, if I can't see the whole sub on my screen at once, it's that much harder to suss out all of the moving parts.

Line 315 or so, you build up a string of random characters manually. Consider any of the many existing (and tested) random string generators on CPAN, such as String::Random:

use String::Random; my $boundary = String::Random->new->randregex('[A-Za-z0-9]{32}');

Finally, I hope this helps, and that none of this comes across as the "one, true way" or overly preachy. These were just a few things that stuck out to me while giving your code a very light skim, and I figured you might appreciate someone shining a light on them for your consideration. Congrats again on being a published Perl author.

use strict; use warnings; omitted for brevity.

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://11107233]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others surveying the Monastery: (5)
As of 2020-04-05 01:56 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?
    The most amusing oxymoron is:
















    Results (33 votes). Check out past polls.

    Notices?