Re: Review CCP::Xero aspiring CPAN module

[stevieb]

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.