Skip to content

#1638 Add Parameter Object pattern#1650

Merged
iluwatar merged 6 commits into
iluwatar:masterfrom
richardmr36:master
Feb 28, 2021
Merged

#1638 Add Parameter Object pattern#1650
iluwatar merged 6 commits into
iluwatar:masterfrom
richardmr36:master

Conversation

@richardmr36
Copy link
Copy Markdown
Contributor

This PR introduces Parameter Object pattern. It has a working example with tests and explanations.

@ohbus ohbus self-assigned this Feb 11, 2021
@ohbus ohbus linked an issue Feb 11, 2021 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

@ohbus ohbus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @richardmr36 for your awesome contribution ⭐

Overall a Very Good PR 🥳

There are a few things I would like you to consider before we can merge this:

  • Few more test cases to increase the coverage of the code.
  • Look into the comments to add a few extra lines.
  • Commenting the Code to understand why certain actions are being taken
  • Logging the output.

Once these changes are updated, you can request another review.

Looking forward to it.

Comment thread parameter-object/etc/parameter-object.urm.puml Outdated
@richardmr36
Copy link
Copy Markdown
Contributor Author

richardmr36 commented Feb 11, 2021

@ohbus Regarding logging the output, there is only one output (in App.java) and it's being logged. Let me know if you are talking about anything specific to log.

@ohbus
Copy link
Copy Markdown
Contributor

ohbus commented Feb 11, 2021

@ohbus Regarding logging the output, there is only one output (in App.java) and it's being logged. Let me know if you are talking about anything specific to log.

It is like when the tests are running, if there are certains logs of how it is performing with the values, it would suffice.

Copy link
Copy Markdown
Contributor

@ohbus ohbus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update.

There are a few minor changes I would like you to go through.

Comment thread parameter-object/src/main/java/com/iluwatar/parameter/object/App.java Outdated
Comment thread parameter-object/src/main/java/com/iluwatar/parameter/object/SearchService.java Outdated
Copy link
Copy Markdown
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks extremely good already, but added just a couple of minor comments

Comment thread parameter-object/README.md Outdated
Comment thread parameter-object/README.md Outdated
Comment thread parameter-object/src/main/java/com/iluwatar/parameter/object/App.java Outdated
Copy link
Copy Markdown
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Since I think all the review comments have been addressed, I'll merge this. Thanks a lot for the good review comments @ohbus 👍🏻

@iluwatar iluwatar self-assigned this Feb 28, 2021
@iluwatar iluwatar added this to the 1.24.0 milestone Feb 28, 2021
@iluwatar iluwatar merged commit 846d056 into iluwatar:master Feb 28, 2021
@iluwatar
Copy link
Copy Markdown
Owner

Many thanks @richardmr36 for the valuable contribution!
@all-contributors please add @richardmr36 for code

@allcontributors
Copy link
Copy Markdown
Contributor

@iluwatar

I've put up a pull request to add @richardmr36! 🎉

@iluwatar
Copy link
Copy Markdown
Owner

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parameter Object idiom

3 participants