Skip to content

Conversation

waisuan
Copy link
Contributor

@waisuan waisuan commented Oct 28, 2015

Hello.

Feel free to review the code that I have committed for the new/mentioned pattern. Do let me know if you discover any issues with the commit.

Thanks!

@iluwatar
Copy link
Owner

iluwatar commented Nov 1, 2015

  • Put under review badge to the pull request
  • Does the example code implement the pattern correctly and follow good coding practices?
  • Does the example code have enough test coverage?
  • Is the example code commented well enough?
  • Is the example code following JavaDoc conventions?
  • Are the project coding conventions being followed?
  • Is the class diagram generated correctly?
  • Is the index.md implemented correctly so the pattern will show correctly on the web site?

Remarks:

  • There is an error in package declaration. The folder structure suggests package of com.wssia.caching but the declaration reads main.java.com.wssia.caching. For consistency I suggest you change the package to com.iluwatar.caching.
  • There seems to be no main method in App. Is it intended to be run only through the JUnit tests? Please note that this is not consistent with the other examples.
  • There is an option to use MongoDB as backend database. However, this is not enabled by default and instead in-memory data structure is used. Apparently MongoDB it is taken to use by setting the boolean flag in AppTest#setUp. I tried this and it seems to work.

Copy link
Owner

Choose a reason for hiding this comment

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

@iluwatar
Copy link
Owner

iluwatar commented Nov 1, 2015

@waisuan I'm done reviewing. Overall the implementation looks good. When you've implemented the changes please comment on this thread and I'll check again.

@waisuan
Copy link
Contributor Author

waisuan commented Nov 1, 2015

Hello @iluwatar. Thanks for the feedback. Kindly check the latest changes and let me know if there is anything else that I have missed out. Thanks.

P.S. And yes, I have left an option to use MongoDB as the backend DB as an alternative. Initially, I was testing with Mongo but it slipped my mind that this approach might cause the global maven compilation to fail when running the tests. So, I set the default DB to an internal structure instead to avoid said issue. Also, you're correct -- setting the flag in AppManager.initDB(boolean useMongoDB) allows the switching of the DBs. Hope that's alright. :)

P.P.S. I am not sure why the code coverage test failed. I did not make any major changes to the code. I looked at the details of the coverage test and there weren't any significant feedback on which modules/classes were causing the decrease in coverage. So...I'm uncertain as to whether my commit was the cause of the decrease or not. Do let me know if there is something I can do to fix this. Thanks.

iluwatar added a commit that referenced this pull request Nov 1, 2015
Issue #273: Caching Patterns [new pattern]
@iluwatar iluwatar merged commit a2dd87d into iluwatar:master Nov 1, 2015
@iluwatar
Copy link
Owner

iluwatar commented Nov 1, 2015

Thank you very much for the valuable contribution!

pratigya0 pushed a commit to pratigya0/java-design-patterns that referenced this pull request Aug 3, 2023
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.

2 participants