Skip to content

Solves #404 Refactor dao #407

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Mar 28, 2016
Merged

Solves #404 Refactor dao #407

merged 7 commits into from
Mar 28, 2016

Conversation

npathai
Copy link
Contributor

@npathai npathai commented Mar 23, 2016

List of changes:

  • Two implementations of CustomerDao interface. DbCustomerDao and InMemoryCustomerDao.
  • CustomerDaoImpl is renamed to InMemoryCustomerDao.
  • Use of Hierarchical Context Runner for making tests more readable.
  • Updated class diagram.
  • Schema is created by App before using DbCustomerDao.

@npathai
Copy link
Contributor Author

npathai commented Mar 23, 2016

@iluwatar Please go through the changes. I have changed test cases a bit to use my style of unit testing. The method names are verbose and communicate the behavior under test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.993% when pulling 3f7ead5 on refactor-dao into aebd857 on master.

iluwatar
iluwatar reviewed Mar 27, 2016
View reviewed changes
dao/pom.xml Outdated
<dependency>
<groupId>com.h2database</groupId>
<artifactId>h2</artifactId>
<scope>compile</scope>
Copy link
Owner

Choose a reason for hiding this comment

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

compile is the default scope so no need to define it

@iluwatar
Copy link
Owner

Review remarks:

  • The SQL for creating/dropping schema is duplicated multiple times here and there. Would it be possible to put this in one place e.g. SQL-file and execute from there?
  • Handling of null values. It's a matter of style and opinion but we could utilize Optional instead.

@iluwatar
Copy link
Owner

@npathai you've got my review comments.

@npathai
Copy link
Contributor Author

npathai commented Mar 27, 2016

@iluwatar

  • Regarding SQL I will try to address it.
  • Regarding Optional, I gave it some thought while designing the interface according to java 8. But left it as is because Optional is useful when null can have two meanings. Such as Map.get(), where it can mean that key was not mapped to any value or key was mapped to a null value. There were a few links as well discussing it. I will attach it. Optional is not a replacement for null.

@npathai
Copy link
Contributor Author

npathai commented Mar 27, 2016

@iluwatar Well after a lot of opinion reading I decided that for the sake of keeping interface fuctional ready, I will change the return type to be Optional.

But in reality I dont hate nulls. If documented correctly they are not a problem. Also creating an optional wrapper creates unnecessary GC pressure if the operation is heavily used.

@npathai
Copy link
Contributor Author

npathai commented Mar 28, 2016

@iluwatar Done with the changes.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 91.008% when pulling 464ae16 on refactor-dao into d406f0f on master.

@iluwatar iluwatar merged commit d20145c into master Mar 28, 2016
@iluwatar
Copy link
Owner

@npathai these are great improvements 👍

@iluwatar iluwatar modified the milestone: 1.11.0 Apr 3, 2016
@iluwatar iluwatar deleted the refactor-dao branch May 22, 2016 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: under review
Projects
None yet
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.
3 participants
@npathai @coveralls @iluwatar