Data Mapper #34

[inbravo]

A basic data-mapper implementation based on discussion on #34

[coveralls]

Coverage remained the same at 91.008% when pulling 06e0a15 on inbravo:master into d631585 on iluwatar:master.

[iluwatar]

Review comments:

• StudentFirstMapper and StudentSecondMapper seem very similar. Can we drop one of them to simplify the example? You wouldn't need to process command line parameters in App after the change.
• In App the conditional use of Logger is confusing. Could we just output the debug log without testing any conditions?
• DataMapperException seems to forward the call to super in most of its methods. In this case you can leave out the implementation because it's the default behavior.
• You could add unit test for Student to verify that equality testing works
• Datamapper module is missing from the main pom.xml

[iluwatar]

Add JavaDoc

[iluwatar]

@inbravo you have my review comments. Please notify me once you've addressed them.

[coveralls]

Coverage remained the same at 91.008% when pulling 8529d6e on inbravo:master into d631585 on iluwatar:master.

[inbravo]

• StudentSecondDataMapper is dropped and StudentFirsDataMapper is renamed to StudentDataMapperImpl for better readability.
• Conditional usage of logger in App class is discarded.
• DataMapperException is forwarding calls to RuntimeException which is a core Exception class but not any reference from implementation. This is helpful in detailing users about removing all data-mapper exception dependency from code. Please review again.
• Student equality test is added at StudentTest
• Your last point is not clear to me; please share more information on it.

I hope just a final review is required.

[coveralls]

Coverage remained the same at 91.008% when pulling 32736fc on inbravo:master into d631585 on iluwatar:master.

[iluwatar]

@inbravo took another look. Here are my comments:

• DataMapperException defines additional constructors that are never used. These should be removed.
• You should add data-mapper as a module to the main pom.xml Look for <modules> declaration and see how the other modules have been added.
• (optional improvement) Since this is a database related pattern it would be great to see actual real database underneath.

Please comment when you're ready for another look.

[inbravo]

Parent info is required, i am giving this, please review, build is failed

<parent>
    <groupId>com.iluwatar</groupId>
    <artifactId>java-design-patterns</artifactId>
    <version>1.7.0</version>
</parent>

[iluwatar]

Looks like you have the wrong version number. Try this:

<parent>
    <groupId>com.iluwatar</groupId>
    <artifactId>java-design-patterns</artifactId>
    <version>1.12.0-SNAPSHOT</version>
</parent>

[inbravo]

• Unused constructors from DataMapperException discarded
• Main build file updated for addition of new module
• You are absolutely correct it should be DB based; My initial check-in was based on DB in fact. Let me see if i can do this ... (i analyzed and it seems that it will be complete redo, now it depends on you to take the final call...)

[coveralls]

Coverage decreased (-0.3%) to 90.713% when pulling af1db79 on inbravo:master into d631585 on iluwatar:master.

[inbravo]

This pattern is ready for release; you can plan out ....

[iluwatar]

@inbravo I've merged the initial implementation. Looking forward to the real database implementation though.