worksap commerce (hybris)refactoring

SAP Commerce (Hybris) Quick coding exercise - Refactoring the DAO

, ,

image from wikimedia.org

You got some legacy code below. So, you’ve got to refactor the code and explain.

The interface

1
2
3
4
5
6
package com.carsupplier.core.daos.carinfo;

public interface CarInfoFinder {
    List<CarModel> getCarBybody(BodyTypeModel bodyTypeModel);
    List<CarInfoModel> getCarForBodyNumber(String bodyNumber);
}

The implementation

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
package com.carsupplier.core.daos.carinfo;

/**
 * TODO refactor the class before a code freeze
 * before the holiday shopping season 2014
 */
public class DraftCarInfoFinder extends DefaultGenericDao<CarInfoModel> implements CarInfoFinder {

    private static final String BODY = "body";
    private static final String BODY_NUMBER = "bodyNumber";

    @Override
    public List<CarInfoModel> getCarBybody(BodyTypeModel bodyTypeModel) {
        Map<String, Object> params = Collections.singletonMap(BODY, bodyTypeModel);
        String queryString = buildQuery(params);
        FlexibleSearchQuery query = new FlexibleSearchQuery(queryString);
        SearchResult<CarInfoModel> searchResult = this.getFlexibleSearchService().search(query);
        return searchResult.getResult();
    }
    
    @Override
    public List<CarInfoModel> getCarForBodyNumber(String bodyNumber){
        Map<String, Object> params = Collections.singletonMap(BODY_NUMBER, bodyNumber);
        String queryString = buildQuery(params);
        FlexibleSearchQuery query = new FlexibleSearchQuery(queryString);
        SearchResult<CarInfoModel> searchResult = this.getFlexibleSearchService().search(query);
        List<CarInfoModel> resList = searchResult.getResult();
        if (resList.size() > 1) {
            throw new AmbiguousIdentifierException("Found " + resList.size() + 
                    " carInfo's with the unique bodyNumber '" + bodyNumber + "'");
        } else {
            return resList.isEmpty() ? null : resList.get(0);
        }
    }


    // Other code
}

Quote from the spring config

1
2
3
4
5
6
7
8
<bean id="DraftCarInfoFinder" 
    class="com.carsupplier.core.daos.carinfo.DraftCarInfoFinder"
    parent="baseDao" />

<alias name="defaultCarInfoService" alias="carInfoService"/>
<bean id="defaultCarInfoService" class="com.carsupplier.core.carinfo.impl.DefaultCarInfoService">
    <property name="carInfoDao" ref="DraftCarInfoFinder"/>
</bean>

It smells like deadlines in the fall of 2014. Let’s take a look at this more carefully.

1. Wrong naming

Looks like this is a DAO. In this case, the official documentation recommends:

  • To name the interface of a DAO with the ending string DAO.
  • The interface’s name should be the same as the service, but the “Service” suffix replaced by “Dao”

So we could try to rename the interface from CarInfoFinder to CarInfoDao for example.

1
2
3
4
5
6
package com.carsupplier.core.daos.carinfo;

public interface CarInfoDao {
    List<CarModel> getCarBybody(BodyTypeModel bodyTypeModel);
    List<CarInfoModel> getCarForBodyNumber(String bodyNumber);
}

What about the implementation? The documentation says that:
The implementations name of DAO should be the same as the DAO interface but with a standard prefix.

DraftCarInfoFinder doesn’t sound very good. To follow the suggestion above we could try to rename the implementation to DefaultCarInfoDao. This name meets the requirements.

1
2
3
4
5
6
package com.carsupplier.core.daos.carinfo;

public class DefaultCarInfoDao extends DefaultGenericDao<CarInfoModel> implements CarInfoDao {

    // Other code
}

2. Wrong package

The package com.carsupplier.core.daos.carinfo doesn’t look that great, also the documentation suggests to put the interfaces of DAOs in a daos package underneath the service it supports.

Alright, let’s move the code from
com.carsupplier.core.daos.carinfo to
com.carsupplier.core.carinfo.daos underneath the service DefaultCarInfoService.

1
2
3
4
5
6
package com.carsupplier.core.carinfo.daos;

public interface CarInfoDao {
    List<CarModel> getCarBybody(BodyTypeModel bodyTypeModel);
    List<CarInfoModel> getCarForBodyNumber(String bodyNumber);
}

What about the implementation? The documentation says that:
The implementation classes of DAO should be located in an impl package.

So, to fix this, we need to move the implementation from
com.carsupplier.core.daos.carinfo to
com.carsupplier.core.carinfo.daos.impl

1
2
3
4
5
6
package com.carsupplier.core.carinfo.daos.impl;

public class DefaultCarInfoDao extends DefaultGenericDao<CarInfoModel> implements CarInfoDao {

    // Other code
}

3. Wrong configuration of the Spring bean

Let’s look at the spring config again:

1
2
3
<bean id="DraftCarInfoFinder" 
    class="com.carsupplier.core.carinfo.daos.impl.DefaultCarInfoDao"
    parent="baseDao" />

What does the documentation say about the Spring configuration for a DAO?

  • The Spring bean id for a DAO should be the same as the implementation
  • The Spring bean id should start with a lowercase letter

As you can see the Spring configuration for the DAO has several issues:

  • The ID has a different name than the implementation class
  • The ID starting with an uppercase letter
    Also, it would be nice to create an alias with the same name as the interface.

To fix this, let’s just rename the ID from DraftCarInfoFinder to defaultCarInfoDao, and create the alias carInfoDao.

1
2
3
4
5
6
7
8
9
<alias name="defaultCarInfoDao" alias="carInfoDao"/>
<bean id="defaultCarInfoDao" 
    class="com.carsupplier.core.carinfo.daos.impl.DefaultCarInfoDao"
    parent="baseDao" />

<alias name="defaultCarInfoService" alias="carInfoService"/>
<bean id="defaultCarInfoService" class="com.carsupplier.core.carinfo.impl.DefaultCarInfoService">
    <property name="carInfoDao" ref="carInfoDao"/>
</bean>

4. Wrong implementation of the interface methods

Next, let’s look at the implementation of the interface methods:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
@Override
public List<CarInfoModel> getCarBybody(BodyTypeModel bodyTypeModel) {
    Map<String, Object> params = Collections.singletonMap(BODY, bodyTypeModel);
    String queryString = buildQuery(params);
    FlexibleSearchQuery query = new FlexibleSearchQuery(queryString);
    SearchResult<CarInfoModel> searchResult = this.getFlexibleSearchService().search(query);
    return searchResult.getResult();
}

@Override
public CarInfoModel getCarForBodyNumber(String bodyNumber){
    Map<String, Object> params = Collections.singletonMap(BODY_NUMBER, bodyNumber);
    String queryString = buildQuery(params);
    FlexibleSearchQuery query = new FlexibleSearchQuery(queryString);
    SearchResult<CarInfoModel> searchResult = this.getFlexibleSearchService().search(query);
    List<CarInfoModel> resList = searchResult.getResult();
    if (resList.size() > 1) {
        throw new AmbiguousIdentifierException("Found " + resList.size() + 
                " carInfo's with the unique bodyNumber '" + bodyNumber + "'");
    } else {
        return resList.isEmpty() ? null : resList.get(0);
    }
}

The documentation says about DAO search methods, the following:

  • Search methods should be named as findByXXXX where XXXX lists the parameters for the method
  • Methods should not return null
  • Use FlexibleSearchService.searchUnique() when searching for a single item
  • Use FlexibleSearchService pagination for potentially-large queries

First, rename methods:

  • from getCarBybody to findCarsByBodyType
  • from getCarForBodyNumber to findCarByBodyNumber
    1
    2
    3
    4
    5
    6
    package com.carsupplier.core.carinfo.daos;

    public interface CarInfoDao {
        List<CarModel> findCarsByBodyType(BodyTypeModel bodyTypeModel);
        CarInfoModel findCarByBodyNumber(String bodyNumber);
    }

Second, use FlexibleSearchService.searchUnique() in the method findCarByBodyNumber:

1
2
3
4
5
6
7
@Override
public CarInfoModel findCarByBodyNumber(String bodyNumber) {
    Map<String, Object> params = Collections.singletonMap(BODY_NUMBER, bodyNumber);
    String queryString = buildQuery(params);
    FlexibleSearchQuery query = new FlexibleSearchQuery(queryString);
    return this.getFlexibleSearchService().searchUnique(query);
}

Finally, use pagination in the method findCarsByBodyType. To do this:

  • Add PageableData as a parameter to the method findCarsByBodyType
  • Configure FlexibleSearchQuery by PageableData
    1
    2
    3
    4
    5
    6
    7
    8
    9
    10
    11
    12
    13
    14
    15
    16
    17
    @Override
    public List<CarInfoModel> findCarsByBodyType(BodyTypeModel bodyTypeModel,
                                                 PageableData pageableData) {
        Map<String, Object> params = Collections.singletonMap(BODY, bodyTypeModel);
        FlexibleSearchQuery query = buildFlexibleSearchQuery(params, pageableData);
        SearchResult<CarInfoModel> searchResult = this.getFlexibleSearchService().search(query);
        return searchResult.getResult();
    }

    private FlexibleSearchQuery buildFlexibleSearchQuery(Map<String, Object> params,
                                                         PageableData pageableData) {
        String queryString = buildQuery(params);
        FlexibleSearchQuery query = new FlexibleSearchQuery(queryString);
        query.setStart(pageableData.getCurrentPage() * pageableData.getPageSize());
        query.setCount(pageableData.getPageSize());
        return query;
    }
    So, the final code looks like this:

The Interface

1
2
3
4
5
6
7
package com.carsupplier.core.daos.carinfo;

public interface CarInfoDao {
    List<CarInfoModel> findCarsByBodyType(BodyTypeModel bodyTypeModel, 
                                          PageableData pageableData);
    CarInfoModel findCarByBodyNumber(String bodyNumber);
}

The Implementation

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
package com.carsupplier.core.daos.carinfo;

public class DefaultCarInfoDao extends DefaultGenericDao<CarInfoModel> implements CarInfoDao {

    private static final String BODY = "body";
    private static final String BODY_NUMBER = "bodyNumber";

    @Override
    public List<CarInfoModel> findCarsByBodyType(BodyTypeModel bodyTypeModel,
                                                 PageableData pageableData) {
        Map<String, Object> params = Collections.singletonMap(BODY, bodyTypeModel);
        FlexibleSearchQuery query = buildFlexibleSearchQuery(params, pageableData);
        SearchResult<CarInfoModel> searchResult = this.getFlexibleSearchService().search(query);
        return searchResult.getResult();
    }

    private FlexibleSearchQuery buildFlexibleSearchQuery(Map<String, Object> params,
                                                         PageableData pageableData) {
        String queryString = buildQuery(params);
        FlexibleSearchQuery query = new FlexibleSearchQuery(queryString);
        query.setStart(pageableData.getCurrentPage() * pageableData.getPageSize());
        query.setCount(pageableData.getPageSize());
        return query;
    }

    @Override
    public CarInfoModel findCarByBodyNumber(String bodyNumber) {
        Map<String, Object> params = Collections.singletonMap(BODY_NUMBER, bodyNumber);
        String queryString = buildQuery(params);
        FlexibleSearchQuery query = new FlexibleSearchQuery(queryString);
        return this.getFlexibleSearchService().searchUnique(query);
    }

    // Other code
}

Quote from the spring config

1
2
3
4
5
6
7
8
9
<alias name="defaultCarInfoDao" alias="carInfoDao"/>
<bean id="defaultCarInfoDao" 
    class="com.carsupplier.core.carinfo.daos.impl.DefaultCarInfoDao"
    parent="baseDao" />

<alias name="defaultCarInfoService" alias="carInfoService"/>
<bean id="defaultCarInfoService" class="com.carsupplier.core.carinfo.impl.DefaultCarInfoService">
    <property name="carInfoDao" ref="carInfoDao"/>
</bean>

Created based on official documentation

Follow me in the group in telegram

Share