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

Review CCP::Xero aspiring CPAN module

by localshop (Monk)
on Nov 24, 2016 at 04:03 UTC ( [id://1176469]=perlquestion: print w/replies, xml ) Need Help??

localshop has asked for the wisdom of the Perl Monks concerning the following question:

Integrating into Xero Accounting System API uses OAuth (v1.0a) and I was able to make initial progress with the CPAN Net::Xero module

I ended up writing my own module that I use internally but was looking to release it into CPAN eventually.

I posted it to GitHub at https://github.com/pscott-au/CCP-Xero and if any generous monks could provide any tips on cleaning it up, improving I would appreciate any feedback.

Replies are listed 'Best First'.
Re: Review CCP::Xero aspiring CPAN module
by marto (Cardinal) on Nov 24, 2016 at 11:41 UTC
Re: Review CCP::Xero aspiring CPAN module
by eyepopslikeamosquito (Archbishop) on Nov 25, 2016 at 06:27 UTC
Re: Review CCP::Xero aspiring CPAN module
by stevieb (Canon) on Nov 24, 2016 at 13:18 UTC

    After a very quick glance... the use_ok tests should only be specified in a single file (00-load.t). Having them in each test file is redundant, and if/when you get up to 30-50 test files (often by copy/pasting, then editing), that's a lot of test calls that are unnecessary.

    Also, in my tests, I prefer to not use the plan tests => N, and instead, forget that and just put done_testing(); at the end of my test. This allows me to not worry about keeping track of my test count (this is just my personal preference though).

    The TEST_CONFIG.tpl file should reside in the t/ directory (perhaps t/data, and be automatically loaded into the env var when needed. This keeps the directory structure clean, and alleviates a user from having to do manual work when running/adding tests.

    Things like this:

    foreach my $key (@PARAMS) { $self->{$key} = $params{$key} || '' }

    ...may be better written like:

    $self->{$key} = defined $params{$key} ? $params{$key} : '';

    The former way will assign an empty string to the value of $self->{key} if $params{$key} has a false value, which may not be what you want (or, you may want to change in the future). The latter way will only assign an empty string if the value is undefined. I could write:

    $self->{$key} = $params{$key} // '';

    ...but you're using minimum perl 5.6, and the defined-or (//) operator wasn't released until (iirc) 5.10

    Here's another personal preference. I often use right-side if statements, but when the line is long, I think the conciseness and ease-of-reading of the statement is lost. ie. it's harder to tell at a glance what the control flow is. I'd rewrite this into a proper structured block:

    return $self->new( %{$data->{Invoices}[0]} ) if ( ref($data->{Invoices +}) eq 'ARRAY' and scalar(@{$data->{Invoices}})==1 );

    return 10 if $x > 5; is an example where it is very easy to see what's happening at a glance.

    I'd also change some of the naming conventions. Perl typically uses snake_case for vars. UPPER case typically means a constant, CamelCase represents a module name etc. So for things like @PARAMS, I'd go with the idiomatic @params.

    Other than what the other monks have already pointed out, most of the rest I've glanced at looks pretty good.

Re: Review CCP::Xero aspiring CPAN module
by 1nickt (Canon) on Nov 24, 2016 at 10:35 UTC

    See also PrePAN, which is intended to be used for what you seek (but can be lightly trafficked).

    My thoughts:

    • Avoid top-level namespace when at all possible
    • Reuse existing namespace where possible
    • Get more specific as you move to the right in the name
    So I might recommend Net::Xero::CPP

    The way forward always starts with a minimal test.

Log In?
Username:
Password:

What's my password?
Create A New User
Domain Nodelet?
Node Status?
node history
Node Type: perlquestion [id://1176469]
Approved by Corion
help
Chatterbox?
and the web crawler heard nothing...

How do I use this?Last hourOther CB clients
Other Users?
Others imbibing at the Monastery: (7)
As of 2024-03-28 19:50 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    No recent polls found