Yevgeny Zaspitsky has posted comments on this change. Change subject: core: Add JPA Java infrastructure ......................................................................
Patch Set 60: (13 comments) https://gerrit.ovirt.org/#/c/33835/60/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/HibernateTemplate.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/HibernateTemplate.java: Line 1: org.ovirt.engine.core.dao This class should go to ...jpa package too Line 19: abstract 1. Moti, Thank you for creating the ascii UML chart. 2. According to UML chart this class has not be abstract. Also (as I mentioned before) we need a singleton factory class for DAOs to create instances of this class, one per DAO/entity type. The EM should be injected there. 3. Once this clas isn't abstract, a unit-testwould be needed. Line 19: HibernateTemplate As sonn as we implement JPA rather than Hibernate (yes very small difference) we might consider calling that JpaTemplate Line 21: protected can this be private? Line 23: type please rename type to entityType Line 23: Class<?> Shouldn't this be Class<T>? IMHO most if not all @SuppressWarnings shoud be redundant after that. Line 29: Class<?> type same here Line 29: public If we gonna create the instances of the class by the factory, the constructor should be package private. Line 59: protected If this class will not be used ppy inheritance, all protected methods should become or public or private (last is preferrable). Line 60: (T) Once type would be Class<T> the casting will become redundant as well as @SuppressWarnings Line 84: Query Using TypedQuery (JPA 2) is type-safe, so that's preferable over Query (applies to all Query usages in the class) Line 107: multiResults This methos would be redundant after starting using TypedQuery Line 115: : @SuppressWarnings("rawtypes") : public List multiResultsNoCast(Query query) { : return multiResultsNoCast(query, false); : } : : @SuppressWarnings("rawtypes") : protected List multiResultsNoCast(Query query, boolean detach) { : List entities = query.getResultList(); : : if (detach) { : for (Object entity : entities) { : em.detach(entity); : } : } : return entities; : } since using TypedQuery "NoCast" suffix is irrelevant for the 2 methods -- To view, visit https://gerrit.ovirt.org/33835 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide82bf8cc647426e37dc42a113867c52699c3f0b Gerrit-PatchSet: 60 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <wallaroo1...@gmail.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
