Skip to content
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

exportcert refactor #252

Merged
merged 8 commits into from
Nov 12, 2018

Conversation

sharan8
Copy link

@sharan8 sharan8 commented Nov 11, 2018

The certificate creation and export can be abstracted out into a new utility class so as to allow for better separation of concerns and scope for future extensions. The organisation is now as follows:

ExportCertCommand:

  • Affirms and assigns the export path based on the ability to create a folder to export to
  • Validates the index input and raises a CommandException if there is a violation
  • Checks if the volunteer has non-zero event records
  • Retrieves the records and corresponding events for the selected volunteer from the model
  • Creates a new CertGenerator object by supplying the save path, volunteer and (record, event) pairs.
  • Calls the generatePdf method of the CertGenerator class and handles the potential resulting IOExceptions.

CertGenerator:

  • Gets instantiated with a save path, volunteer object and volunteer data in the from of (record, event) pairs.
  • Populates a PDF document with the supplied volunteer information
  • Exports the PDF document to the supplied savepath

Some design considerations:

  • Save path is supplied by ExportCertCommand so as to support future functionality to get the filepath from the user itself. As such, CertGenerator is excluded from the save path decision making process.
  • The retrieval of volunteer information is done in ExportCertCommand itself so as to prevent the need for both CertGenerator and ExportCertCommand to interact with the model. This is inspired by the Law of Demeter.

@sharan8 sharan8 added this to the v1.4 milestone Nov 11, 2018
@sharan8 sharan8 added the status.Ongoing The issue is currently being worked on. note: remove this label before closing an issue. label Nov 11, 2018
@coveralls
Copy link

coveralls commented Nov 11, 2018

Coverage Status

Coverage increased (+0.09%) to 67.353% when pulling b15d821 on sharan8:exportcert-refactor into 76ad597 on CS2103-AY1819S1-W16-2:master.

@sharan8 sharan8 added status.Ready The PR is ready to be reviewed. note: remove this label before merging a PR. status.Ongoing The issue is currently being worked on. note: remove this label before closing an issue. and removed status.Ongoing The issue is currently being worked on. note: remove this label before closing an issue. status.Ready The PR is ready to be reviewed. note: remove this label before merging a PR. labels Nov 11, 2018
@sharan8 sharan8 added status.Ready The PR is ready to be reviewed. note: remove this label before merging a PR. and removed status.Ongoing The issue is currently being worked on. note: remove this label before closing an issue. labels Nov 12, 2018
@iMarbles iMarbles merged commit 3d112ae into CS2103-AY1819S1-W16-2:master Nov 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status.Ready The PR is ready to be reviewed. note: remove this label before merging a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants