worksap commerce (hybris)refactoring

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

, ,

image from wikimedia.org

In the morning you found a new issue in the issue tracking software. The problem is that sometimes a NullPointerException occurs in the StrategyOfGettingDescriptor.

StrategyOfGettingDescriptor.java

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
package com.carsupplier.system.strategy.descriptor;

public class StrategyOfGettingDescriptor {
    private DescriptorDAO descriptorDAO;

    public DescriptorModel findDescriptor(final ConfigModel configModel) {

        if (configModel == null) {
            throw new SystemException("configModel is null!");
        }

        final String configKey = configModel.getKey();
        final String configGroup = configModel.getGroup();

        List<DescriptorModel> result = descriptorDAO.findByGroupAndKey(configGroup, configKey);
        if (result.size() > 1) {
            throw new SystemException(result.size() + " descriptors found!");
        }
        return result.get(0);
    }
}

DescriptorDao.java

1
2
3
4
5
6
7
public interface DescriptorDao {
    /**
     * @return the List of DescriptorModels. 
     * If no result found then return an empty list.
     */
    List<DescriptorModel> findByGroupAndKey(String configGroup, String configKey);
}

Quote from the spring-config

1
2
3
4
<bean id="strategyForDescriptor" 
    class="com.carsupplier.core.car.services.StrategyOfGettingDescriptor">
    <property name="descriptorDAO" ref="descriptorDAO"/>
</bean>

This is a problem on line 19: DescriptorDAO returns an empty list sometimes and an attempt of getting the first element of the list throws an exception. So, let’s fix NullPointerException quickly:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
package com.carsupplier.system.strategy.descriptor;

public class StrategyOfGettingDescriptor {
    private DescriptorDAO descriptorDAO;

    public DescriptorModel findDescriptor(final ConfigModel configModel) {

        if (configModel == null) {
            throw new SystemException("configModel is null!");
        }

        final String configKey = configModel.getKey();
        final String configGroup = configModel.getGroup();

        List<DescriptorModel> result = descriptorDAO.findByGroupAndKey(configGroup, configKey);
        if (result.isEmpty()) {
            throw new SystemException("No descriptors found!");
        } else if (result.size() > 1) {
            throw new SystemException(result.size() + " descriptors found!");
        }
        return result.get(0);
    }
}

Does it look well? Maybe.

1. Inappropriate exceptions

So, let’s check what the official documentation says:

  • If expecting a single value, throw AmbiguousIdentifierException when more then one value is found
  • Throw UnknownIdentifierException when no value is found.

To follow the official recommendations we have to:

  • Replace SystemException with AmbiguousIdentifierException if more than one descriptor is found (line 19)
  • Replace SystemException with UnknownIdentifierException if no descriptor is found (line 17)
    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
    package com.carsupplier.system.strategy.descriptor;

    import de.hybris.platform.servicelayer.exceptions.SystemException;

    public class StrategyOfGettingDescriptor {
        private DescriptorDAO descriptorDAO;

        public DescriptorModel findDescriptor(final ConfigModel configModel) {

            if (configModel == null) {
                throw new SystemException("configModel is null!");
            }

            final String configKey = configModel.getKey();
            final String configGroup = configModel.getGroup();

            List<DescriptorModel> result = descriptorDAO.findByGroupAndKey(configGroup, configKey);
            if (result.isEmpty()) {
                throw new UnknownIdentifierException("No descriptors found!");
            } else if (result.size() > 1) {
                throw new AmbiguousIdentifierException(result.size() + " descriptors found!");
            }
            return result.get(0);
        }
    }
    What’s about line 11? The code throws SystemException if the parameter of the method is incorrect. The documentation says:
  • Throw IllegalArgumentException if method parameters are incorrect.

To follow this recomendation we can use de.hybris.platform.servicelayer.util.ServicesUtil.validateParameterNotNull. This method uses a standard google library to check the parameter:

1
2
3
4
5
public static void checkArgument(boolean expression, @Nullable Object errorMessage) {
  if (!expression) {
    throw new IllegalArgumentException(String.valueOf(errorMessage));
  }
}
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
package com.carsupplier.system.strategy.descriptor;

public class StrategyOfGettingDescriptor {
    private DescriptorDAO descriptorDAO;

    public DescriptorModel findDescriptor(final ConfigModel configModel) {

        validateParameterNotNull(configModel, "configModel is null!");

        final String configKey = configModel.getKey();
        final String configGroup = configModel.getGroup();

        List<DescriptorModel> result = descriptorDAO.findByGroupAndKey(configGroup, configKey);
        if (result.isEmpty()) {
            throw new UnknownIdentifierException("No descriptors found!");
        } else if (result.size() > 1) {
            throw new AmbiguousIdentifierException(result.size() + " descriptors found!");
        }
        return result.get(0);
    }
}

What else?

2. Wrong Design. Wrong configuration

As you may have learned from my previous articles, you can see that StrategyOfGettingDescriptor has several problems:

  • Wrong design. The Strategy should implement an interface.
  • Wrong name. The interface and the implementation name should have the prefix “Strategy”.
  • Wrong package. The interface should located in strategies sub-package. The implementation in impl sub-package.
  • Wrong bean id and alias name. The alias name should have the same name as the interface. The bean id should the same name as the implementation. Both should start with a lowercase letter.

To fix this:

  • Create an interface and implement
  • Give an appropriate name
1
2
3
4
5
package com.carsupplier.system.strategy.descriptor;

public interface GettingDescriptorStrategy {
    DescriptorModel findDescriptor(final ConfigModel configModel);
}
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
package com.carsupplier.system.strategy.descriptor;

public class DefaultGettingDescriptorStrategy implements GettingDescriptorStrategy {
    private DescriptorDao descriptorDAO;

    @Override
    public DescriptorModel findDescriptor(final ConfigModel configModel) {

        validateParameterNotNull(configModel, "configModel is null!");

        final String configKey = configModel.getKey();
        final String configGroup = configModel.getGroup();

        List<DescriptorModel> result = descriptorDAO.findByGroupAndKey(configGroup, configKey);
        if (result.isEmpty()) {
            throw new UnknownIdentifierException("No descriptors found!");
        } else if (result.size() > 1) {
            throw new AmbiguousIdentifierException(result.size() + " descriptors found!");
        }
        return result.get(0);
    }
}

Next, change the package:

  • for the interface: from com.carsupplier.system.strategy.descriptor to com.carsupplier.system.descriptor.strategy
  • for the implementation: from com.carsupplier.system.strategy.descriptor to com.carsupplier.system.descriptor.strategy.impl
1
2
3
package com.carsupplier.system.descriptor.strategy;

public interface GettingDescriptorStrategy {
1
2
3
package com.carsupplier.system.descriptor.strategy.impl;

public class DefaultGettingDescriptorStrategy implements GettingDescriptorStrategy {

Next, fix the spring-config:

  • create an alias
  • Give appropriate names for the alias and the bean
1
2
3
4
5
<alias name="defaultGettingDescriptorStrategy" alias="gettingDescriptorStrategy"/>
<bean id="defaultGettingDescriptorStrategy" 
    class="com.carsupplier.system.descriptor.strategy.impl.DefaultGettingDescriptorStrategy">
    <property name="descriptorDAO" ref="descriptorDAO"/>
</bean>

So, everything looks well. The final code below:

The interface

1
2
3
4
5
package com.carsupplier.system.descriptor.strategy;

public interface GettingDescriptorStrategy {
    DescriptorModel findDescriptor(final ConfigModel configModel);
}

The implementation

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
package com.carsupplier.system.descriptor.strategy.impl;

public class DefaultGettingDescriptorStrategy implements GettingDescriptorStrategy {
    private DescriptorDao descriptorDAO;

    @Override
    public DescriptorModel findDescriptor(final ConfigModel configModel) {

        validateParameterNotNull(configModel, "configModel is null!");

        final String configKey = configModel.getKey();
        final String configGroup = configModel.getGroup();

        List<DescriptorModel> result = descriptorDAO.findByGroupAndKey(configGroup, configKey);
        if (result.isEmpty()) {
            throw new UnknownIdentifierException("No descriptors found!");
        } else if (result.size() > 1) {
            throw new AmbiguousIdentifierException(result.size() + " descriptors found!");
        }
        return result.get(0);
    }
}

The spring-config

1
2
3
4
5
<alias name="defaultGettingDescriptorStrategy" alias="gettingDescriptorStrategy"/>
<bean id="defaultGettingDescriptorStrategy" 
    class="com.carsupplier.system.descriptor.strategy.impl.DefaultGettingDescriptorStrategy">
    <property name="descriptorDAO" ref="descriptorDAO"/>
</bean>

Created based on official documentation

Follow me in the group in telegram

Share