Yevgeny Zaspitsky has posted comments on this change.

Change subject: db: aggregate qos and storage qos impl
......................................................................


Patch Set 6: Code-Review-1

(5 comments)

I'd like you implement Mappers separately from the DAO's (the utopia).

http://gerrit.ovirt.org/#/c/27094/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/BaseQosDaoFacadeImpl.java
File 
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/BaseQosDaoFacadeImpl.java:

Line 21:     }
Line 22: 
Line 23:     /**
Line 24:      * @param rs
Line 25:      * @return specific mapper for derived QoS dao
that's lie
Line 26:      * @throws SQLException
Line 27:      */
Line 28:     protected abstract T createPartialQosEntity(ResultSet rs) throws 
SQLException;
Line 29: 


Line 24:      * @param rs
Line 25:      * @return specific mapper for derived QoS dao
Line 26:      * @throws SQLException
Line 27:      */
Line 28:     protected abstract T createPartialQosEntity(ResultSet rs) throws 
SQLException;
The method belongs to a Mapper class rather than to the DAO class.
Line 29: 
Line 30:     /**
Line 31:      * @param map
Line 32:      * @param obj


Line 27:      */
Line 28:     protected abstract T createPartialQosEntity(ResultSet rs) throws 
SQLException;
Line 29: 
Line 30:     /**
Line 31:      * @param map
please add parameters description or remove the javadoc
Line 32:      * @param obj
Line 33:      * @return parameters mapper for derived QoS dao
Line 34:      */
Line 35:     protected abstract void 
updatePartialParametersMapper(MapSqlParameterSource map, T obj);


Line 29: 
Line 30:     /**
Line 31:      * @param map
Line 32:      * @param obj
Line 33:      * @return parameters mapper for derived QoS dao
the method returns void
Line 34:      */
Line 35:     protected abstract void 
updatePartialParametersMapper(MapSqlParameterSource map, T obj);
Line 36: 
Line 37:     /**


Line 52:     }
Line 53: 
Line 54:     @Override
Line 55:     protected RowMapper<T> createEntityRowMapper() {
Line 56:         return new RowMapper<T>() {
There is no reason for creating a Mapper instance on every call to the DAO. The 
Mapper here is stateless.
Line 57:             @Override
Line 58:             public T mapRow(ResultSet rs, int rowNum)
Line 59:                     throws SQLException {
Line 60:                 T entity = createPartialQosEntity(rs);


-- 
To view, visit http://gerrit.ovirt.org/27094
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9af59277b5055453159f002f19046c0051d63b
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Kobi Ianko <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: Yevgeny Zaspitsky <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to