New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed changes to the method get_page_size() in PDF::Create #5

Merged
merged 2 commits into from Oct 14, 2015

Conversation

Projects
None yet
2 participants
@manwar
Owner

manwar commented Oct 6, 2015

Hi Gabor,

I have found few issues with the method get_page_size() in the package PDF::Create as below:

Line 547: $name = lc(shift);

What if the parameter to the method get_page_size() is undef, we are performing unnecessary action lc() on it.

Line 566: if ( !$pagesizes{ uc($name) } ) {

Also what is the point of doing lc() earlier when we are doing uc() here.

Line 570: $pagesizes{ uc($name) };

We are doing uc() again, which is repeating our self/

My proposed solution (with unit test, of course) are as below:

  • Accept the parameter as it arrives i.e $name = shift;
  • Check if it is defined then uc($name) and compare against the keys defined in %pagesize. Croak if it is invalid name.
  • In case it is not defined then assign the default value 'A4'
  • Finally return $pagesizes{$name};

Also the pod document missing one page name i.e. 'A4L'. I have updated it. Last but not the least, I replaced the word "required" with "optional" as the parameter to the method get_page_size () is optional and not required.

Please review the changes.

Many Thanks.

Best Regards,
Mohammad S Anwar

manwar added some commits Oct 6, 2015

Proposed changes to the method get_page_size() in the package PDF::Cr…
…eate

- Accept the parameter as it arrives i.e $name = shift;
- Check if it is defined then uc($name) and compare against the keys defined in %pagesizes. Croak if it is invalid name.
- In case it is not defined then assign the default value 'A4'.
- Finally return $pagesizes{$name};

szabgab added a commit that referenced this pull request Oct 14, 2015

Merge pull request #5 from Manwar/validate-method-get-page-size-params
Proposed changes to the method get_page_size() in PDF::Create

@szabgab szabgab merged commit ffe0348 into manwar:master Oct 14, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@szabgab

This comment has been minimized.

Show comment
Hide comment
@szabgab

szabgab Oct 14, 2015

Collaborator

thanks

Collaborator

szabgab commented Oct 14, 2015

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment