HCL Commerce ts-app customization bad practices - part 1 (framework)

As an architect involved in numerous HCL Commerce projects, I have had the opportunity to observe and evaluate the work of various development teams. In this article, I aim to highlight some frequent programming errors encountered during customization of the transaction server and provide advice on how to avoid them, enabling you as a commerce developer to produce better code.

This article will concentrate solely on HCL Commerce development aspects, excluding general Java coding, testing, or CI/CD best practices.

The first category of bad practices involves not adhering to HCL Commerce architecture. As a reminder, at a very high level in the transaction server, there is typically a REST API resource that calls either a data bean or a controller command. A service backed by a data bean supports read transactions, while a service backed by a controller command supports write transactions. It's a fairly simple and easy to understand concept, however from time to time I see developers not applying it in a correct way. This article marks the beginning of a series where I will primarily focus on the consequences of not adhering to the architecture of the system. In subsequent articles, I will highlight common mistakes that can negatively impact performance and security.

Using controller commands as data providers

A common pitfall I often see is treating a controller command as a "data bean" and using it for pure read operations. Here are some two very simple examples how it may look like.

public class ADataBeanLikeCmdImpl extends ControllerCommandImpl implements ADataBeanLikeCmd {
  private String property1;
  private String property2;
  
  @Override
  public void performExecute() throws ECException {
    //in a real word example over here you will normally a more complex   code
    this.property1 = "Value1";
    this.property2 = "Value2";
  }

  @Override
  public String getProperty1() {
    return property1;
  }

  @Override
  public String getProperty2() {
    return property2;
  }
}

public class ADataBeanLikeCmdImpl extends ControllerCommandImpl implements ADataBeanLikeCmd {

  @Override
  public void performExecute() throws ECException {
  //in a real world example over here you will normally a more complex code
    this.getResponseProperties().put("property1", "Value1");
    this.getResponseProperties().put("property2", "Value1");
  }
}

In both examples, you'll notice values returned by controller commands that can be made available to a caller. One might wonder, "What's the issue? It returns data structures that can be converted to JSON object." Indeed, the code functions correctly, but the primary concern is the potential confusion it may cause. Other developers might question the necessity of using a command over a data bean for a specific case. In my experience, I've found no compelling justification for choosing a controller command over a data bean in this cases. The amount of code isn't reduced (as demonstrated in the Data Bean example below); in fact, it increases since an interface must be created. This approach also restricts flexibility in implementing access control policies and leads to the additional (previously mentioned) confusion. So avoid doing it and for read operations always prefer data beans over controller commands.

public class ADataBean extends SmartDataBeanImpl {
  private String property1;
  private String property2;
  
  @Override
  public void populate() throws ECException {
    this.property1 = "Value1";
    this.property1 = "Value2";
  }

  public String getProperty1() {
    return property1;
  }

  public String getProperty2() {
    return property2;
  }

}

There's an exception from that rule if you have to deal with SOI or SOA modules (they use 'fetch' and 'compose' commands) - but it's slightly different programming model and in majority of your customization you'll deal with data beans.

Adding business logic to REST API resource classes

The second bad practice that I saw from time to time was adding logic to REST resource classes instead of placing them in customized commands or data beans. In the example below we are extending a checkout API by adding some of custom code into the rest resource handler directly.

 public class XCartHandler extends CartHandler {
  @Override
  @javax.ws.rs.POST
  @javax.ws.rs.Path(value="@self/checkout")
  @javax.ws.rs.Produces( value ={"application/json","application/xml","application/xhtml+xml","application/atom+xml"})
  public Response checkOut( @javax.ws.rs.PathParam(value="storeId") String storeId, @javax.ws.rs.QueryParam(value="responseFormat") String responseFormat) {
    if (someConditionMet()) {
      return someCustomCode(storeId, responseFormat);
    } else {
      return super.checkOut(storeId, responseFormat);
    }
  }

  private Response someCustomCode(String storeId, String responseFormat) {
    ...
  }

  private boolean someConditionMet() {
    ...
  }
}

I consider it a bad practice primarily because business logic should not reside in a handler. This can lead to the logic being missed by others, resulting in duplicated code and potential regression bugs. Incorporating business logic into REST handlers also restricts code reusability - for example, if you need to execute the same business logic as part of a scheduled job or expose it through a SOAP web service. A more effective method of extending that logic is presented below.

public class XOrderPrepareCmdImpl extends OrderPrepareCmdImpl {
  @Override
  public void performExecute() throws ECException {
    if(someConditionMet()){
      someCustomCode();
    }
    else{
      super.performExecute();
    }
  }

  private void someCustomCode() {
    ...
  }

  private boolean someConditionMet() {
    ...
  }
}

Of course there might be situations where you'll have to add some code to an API resource class but you want the API resource classes to simply do the absolute minimum to pass processing to a data bean or controller command.

Adding "transactional" code do data beans

This is something that I don't see that often but still happens (usually if a data bean is called by a controller command). Imagine that you have an existing data bean that returns some information about an order. You use that data bean on some page or that bean is consumed by a controller command, now your task is to update a value of an order when that page is opened. Since you already have an existing bean that uses an OrderAccessBean you think it may be the easiest to simply update the order over there (see order.setField(0) in the code below).

public class ADataBean extends SmartDataBeanImpl {
  private String property1;
  private String property2;
  private String orderId;

  @Override
  public void populate() throws ECException {
    this.property1 = "Value1";
    OrderAccessBean order = new OrderAccessBean();
    order.setInitKey_orderId(orderId);
    order.setField1(0);
    this.property2 = order.getDescription();
  }

  public String getProperty1() {
    return property1;
  }

  public String getProperty2() {
    return property2;
  }

  public void setOrderId(String orderId){
    this.orderId = orderId;
  }

}

Will it update the value? Yes, it’ll. So what’s wrong with it? It’s mixing code that changes data with a code that displays data. What if your use case changes and together with an order you'll need to update a value in an order item? You see a point here, right? So how it should have be handled? My advice is always try to separate responsibilities. In our case what you want to do is to move the code that updates the value of Field1 of an order to a new controller command (API).

There’s one more problem with the above code that’ll discuss later in the series.

Not validating data in the right place

A controller command has a validateParameters() method, right? But I saw on many occasions that a validation code is placed elsewhere (especially in the performExecute() method). Let's have a look on the two code snippets from a hypothetical controller command.

@Override
public void performExecute() throws ECException {
  if(someParameter.length()>20){
    doSomething();
  }
  else{
    throw new ...
  }
}

private String someParameter;
	
@Override
public void validateParameters() throws ECException {
  if(someParameter.length()>20){
    throw new ...
  }
}
	
@Override
public void performExecute() throws ECException {
  doSomething();
}

In the first example performExecute() contains both validation and business logic making the code hard to read and it'll be even more complex if you add add more logic to it. In the second example code is much easier to follow as we have put the validation logic in the method that is dedicated for that purpose.

Forgetting to assign default values to optional parameters

This is a kind of mistake that is not captured early (during a code review or testing) will result in a production bug. Imagine that you have a functionality that may accept an optional value. In a controller command you read the value from request properties.

private Integer optionalParameter;
	
@Override
public void setRequestProperties(TypedProperty reqProperties) throws ECException {
  optionalParameter = reqProperties.getInteger("optionalParam");
}  

If the 'optionalParam' parameters is not present in the request properties you'll get an exception. In order to fix this remember to assign the default value to an optional parameter.

private final int OPTIONAL_PARAM_DEFAULT=1;
private Integer optionalParameter;

@Override
public void setRequestProperties(TypedProperty reqProperties) throws ECException {
  optionalParameter = reqProperties.getInteger("optionalParam", OPTIONAL_PARAM_DEFAULT);
}  

Summary

In this post, I have presented the issues most commonly encountered when applying the ts-app customization framework concepts and how to avoid them.

In the upcoming articles of this series, I will disclose some of the most frequent issues affecting the security and performance of HCL Commerce customization.

Should you believe that your team could benefit from a code audit or customized technical enablement, please do not hesitate to contact us.