Data Class code smell? Java object is modified by multiple methods in a row

Question

I am sending a series of SOAP messages to a server and capturing the results of the SOAP transaction. The issue that I am having is that I need to perform five SOAP calls, and each call requires some data from the previous call. This has led me to create a Data Class that holds Strings accessible with Getters and Setters that gets modified after each call. This code seems "smelly" to me, there is a lot of state change going on in the class and I'm just wondering if there is a better way to go about this.

Here is what's going on, I have the following method in an abstract class:

protected SoapBodyParameters soapBodyParameters;

public void release(String primaryConfigurationIdentifier) throws ReleaseManagerException {
    this.soapBodyParameters.setPrimaryConfigurationItemIdentifier(primaryConfigurationIdentifier);

    // Implemented by child - Will throw an exception if bad configuration
    verify();

    // Get user full name and append soapBodyParameters
    String userFullName = sendGetUserInfoSoapMessage(this.soapBodyParameters);
    this.soapBodyParameters.setUserFullName(userFullName);

    // Create a release package on the server - no return needed
    sendCreateReleasePackageFromLocalFilesSoapMessage(this.soapBodyParameters);

    // Find the release package ID we just made and append soapBodyParameters
    String releasePackageSystemId = sendGetReleasePackagesSoapMessage(this.soapBodyParameters);
    this.soapBodyParameters.setReleasePackageSystemId(releasePackageSystemId);

    // Open the release package and find the task model information
    TaskModel taskModel = sendOpenReleasePackageSoapMessage(this.soapBodyParameters);
    this.soapBodyParameter.setTaskModelFile(taskModel.getIARTaskModel());
    this.soapBodyParameters.setTaskModelId(taskModel.getTaskModelId());

    // Implemented by child - Update the task model with required info
    updateLocalTaskModel(this.soapBodyParameters);

    // Save the task model to the release package on the server
    sendSaveTaskModelSoapMessage(this.soapBodyParameters);
}

So with each of these calls modifying soapBodyParameters and each following call relying on the previous call to modify soapBodyParameters.... I don't know, it just feels smelly. Like I am faking a global parameter in the class.

Is there a better way to do this? I'm trying to get better at recognizing when and where a design pattern would be more appropriate.

Answer 1

The code smell I can detect is that your operations require some data from previous calls but you are passing all data from previous calls to them. If you are not sending the full user name in sendOpenReleasePackageSoapMessage(...) you shouldn't include it in the method parameters either. Split the SoapBodyParameters data class to CommonSoapBodyParameters that are sent with all requests and several operation specific data classes that contain only the parameters relevant to the specific operation.

Passing the member field soapBodyParameters as a method parameter is redundant because the methds can already accss the field directly. So once you have created the CommonSoapBodyParameters class, leave that as a protected member field (or better yet, protect it and provide an accessor method getCommonSoapBodyParameters() with which the concrete subclasses can retrieve it) and only pass the operation specific data objects as method parameter.

Answer 2

Basically, whenever you find yourself doing something like:

MyDataStructure data;

call1(data); // modifies data
call2(data); // modifies data
call3(data); // modifies data

you have a big indicator, that the data structure should indeed be a class:

MyDataClass dataClass;

dataClass.call1();
dataClass.call2();
dataClass.call3();

If the call chain depends on a given order, encapsulate this in the class as well:

MyDataClass {

   public void doComplexTask() {
       call1();
       if (resultIsNotGood) throw Exception...
       
       call2();
       ...
       // you get it
   }

   private call1() { ... } // not exposed any more
}