3
\$\begingroup\$

I had to populate the fields of a Service object using a Set (HashSet) with any configuration parameters -> ConfigurationMap (Class with 2 attributes: key and value). The problem is that the attribute quantity of Service object can be set from two distinct parameters ("service_quantity" or "publications_amount") based on the value of another parameter("type").

I've gotten it to work with the code below, but I think it's pretty messy and inefficient. Is there any way to make it more readable considering that the methods signatures are invariable?

@Override
protected void populateServiceConfigurationData(Service theService, Set<ConfigurationMap> configurationParams) {
    boolean publishService = false;
    for (ConfigurationMap configParam : configurationParams) {
        String paramValue = configParam.getValue();
        String paramKey = configParam.getKey();
        setFieldValue(theService, paramKey, paramValue);
        if("type".equals(paramKey) && paramValue.equals("20")) {
            publishService = true;
        }
    }

    if(publishService) {
        for (ConfigurationMap configParam : configurationParams) {
            String paramKey = configParam.getKey();
            if(paramKey.equals("publications_amount")) {
                String paramValue = configParam.getValue();
                setFieldValue(theService, paramKey, paramValue);
            }
        }
    }
}

@Override
public void setFieldValue(Service service, String paramKey, String paramValue) {
    if ("type".equals(paramKey)) {
        service.setType(Integer.valueOf(paramValue));
    }
    if ("service_quantity".equals(paramKey) || "publications_amount".equals(paramKey)) {
        service.setQuantity(Integer.valueOf(paramValue));
    }
    if ("gateway_id".equals(paramKey)) {
        service.setGatewayId(paramValue);
    }
    if ("comissions".equals(paramKey)) {
        service.setCommissions(paramValue);
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Tough one, hard to gain anything from introducing functions. I thought about introducing a Set<String, Function> or similar which maps from parameter names to setters on the service, but even that would add quite some overhead which is hardly worth the effort. I mean, if you're handling only these field names. If you have a hundred or so field names, I think the gains might be worth it to introduce such an infrastructure. \$\endgroup\$ – Bobby Mar 12 at 19:57
  • \$\begingroup\$ What you could do is find and store the value of "publications_amount" in the first loop, alongside "publish_service" and then set it inside the "publish"-loop directly. Then you don't need to do a second loop and the logic becomes a little bit easier to read, you can immediately see that "publications_amount" wins over "service_quantity" (or anything else) when publishing. \$\endgroup\$ – Bobby Mar 12 at 19:58
  • 1
    \$\begingroup\$ Doesn't the first loop already set "publications_amount" - and if so what is it for then (it's redundant)?? \$\endgroup\$ – Mr R Mar 13 at 11:29
2
\$\begingroup\$

I think it's pretty messy and inefficient

Let's say that it is getting messy, especially if the number of parameters (and hidden rules) keeps growing.

About being inefficient, I am not sure. Even if the input is very large, the method populateServiceConfigurationData runs in \$O(N)\$, so I don't think it will be a big problem.

My suggestions:

  • Readability: setting the quantity of the service depends on more than one parameter. So the "rule" to set quantity would be more evident if included in a single function, like setServiceQuantity.
  • Performance: the input set can be converted to a map, to easily access the parameters. The performance gain is minimal, but it should help to make the code more clear.
  • Design: using setFieldValue in populateServiceConfigurationData seems unnecessary. The method populateServiceConfigurationData can be completely independent to setFieldValue. More on this later.
public void populateServiceConfigurationData(Service service, Set<ConfigurationMap> configurationParams) {
    // Convert the input set to a map
    Map<String, String> params = configurationParams.stream()
            .collect(Collectors.toMap(ConfigurationMap::getKey, ConfigurationMap::getValue));

    if (params.containsKey("type")) {
        service.setType(Integer.valueOf(params.get("type")));
    }

    if (params.containsKey("gateway_id")) {
        service.setGatewayId(params.get("gateway_id"));
    }

    if (params.containsKey("comissions")) {
        service.setCommissions(params.get("comissions"));
    }

    setServiceQuantity(service, params);
}

private void setServiceQuantity(Service service, Map<String, String> params){
    boolean isPublishService = params.get("type").equals("20");
    String quantity = isPublishService ? params.get("publications_amount") : params.get("service_quantity");
    service.setQuantity(Integer.valueOf(quantity));
}

(Note: not tested, it's just to give an idea)

Now the logic for setting each property of the service is clear and can be easily extracted into functions like setServiceQuantity.

One issue is that populateServiceConfigurationData needs to know how to set the properties into the service (for example, parsing type to Integer). If you don't like this approach, the setters can be replaced with setFieldValue (as before) or let service do the parsing in its own methods.

The method setFieldValue looks fine, the only thing I can suggest is to use else if since only one condition will match per invocation.

\$\endgroup\$
2
  • \$\begingroup\$ I like your approach but what if the number of parameters was higher in the future. Suppose there are 20 parameters, how this solution would scale? \$\endgroup\$ – jpestana Mar 15 at 7:38
  • 1
    \$\begingroup\$ It's still possible to loop over the map and set the fields of the service on the way, then calling setServiceQuantity at the end. Or I would look at ModelMapper that allows the mapping between a map and a bean. Anyway, if the sole purpose of this class is to configure the service, it is ok to be a bit longer, as long as it is simple and easy to change. \$\endgroup\$ – Marc Mar 15 at 9:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.