Would it be better to split it into several, smaller methods?
In your context, definitely. A cursory look at your method yield the following pseudo-code:
For each process in processList
Create a new VisualProcess instance with some of process's properties
For each step in this process
Create a VisualProcessStep instance with some of step's properties
If this process has an associated Activity, set it to the VisualProcessStep instance
Create VisualProcessStepConnection instances between the current step
and its next steps and add them to a List called connections
Add the VisualProcessStep instance to a List called steps
Add steps and connections to the VisualProcess instance
Add the VisualProcess instance to a List called vProcesses
Each step can be a smaller method in itself. The developer benefits of having smaller methods in your case is that it clearly isolates what fields/properties are required for each 'stage', which in turn allows you to better understand how you code works or can be rearranged. The JVM benefits of having smaller methods is that the JVM can decide when and how to inline method calls optimally to enhance the overall performance.
My suggestions below are going to be Java 8-based, primarily because I like to use such opportunities to showcase how much simpler can external iterations be done using Streams. If you happen to be on older Java versions, the conversion shouldn't be too hard in any case, and I'm quite sure a quick check at StackOverflow should offer some help. ;)
Let's start by starting from the end of the steps covered in the pseudo-code...
Create VisualProcessStepConnection instances between the current step and its next steps
In your current code, you are iterating through the next steps of the current ProcessStep and then constructing a new VisualProcessStepConnection object for each of them. To simplify the creation of the object, you can either give it a constructor that takes in two ProcessStep instances, or use a static method to do so. The suggestion below combines both approaches:
public class VisualProcessStepConnection {
public static of(ProcessStep from, ProcessStep to) {
return new VisualProcessStepConnection(from.getId(), to.getId());
}
}
In turn, you can then create a getConnections() method for ProcessStep that can iterate through its getNextSteps() (assuming it's one of the Collection classes, like a List) to generate the desired output:
public class ProcessStep {
public List<VisualProcessStepConnection> getConnections() {
getNextSteps().stream().map(v -> VisualProcessStepConnection.of(this, v))
.collect(Collectors.toList());
}
}
This takes each element of getNextSteps(), map() it into the required VisualProcessStepConnection object, and then collect() each of the result into a List.
With these two methods, you can already replace the for-loop as such:
connections.addAll(processStep.getConnections());
/*
for (ProcessStep nextProcessStep : processStep.getNextSteps()) {
VisualProcessStepConnection vpsc = new VisualProcessStepConnection();
vpsc.setFrom(processStep.getId());
vpsc.setTo(nextProcessStep.getId());
connections.add(vpsc);
}
*/
If this process has an associated Activity, set it to the VisualProcessStep instance
Currently, you are looping through process.getActivities() to see if there is a matching activity. I will suggest creating a new method that also accepts the activity ID to look for (assuming it has its own type, e.g. ActivityId):
public class Process {
public Optional<Activity> getActivityForId(ActivityId activityId) {
return getActivities().stream().filter(v -> v.getId().equals(activityId))
.findFirst();
}
}
This filter() on your getActivities() uses the same current condition, and optionally returns the first element found. This Optional<Activity> wrapper-object can then be utilized to set the activity-related properties on a VisualProcessStep, provided you have a method there too that can perform the operation on an Activity instance:
public class VisualProcessStep {
private void setActivity(Activity activity) {
setActivityName(activity.getName());
setActivityRealization(activity.getRealization());
}
}
When you combine both, you get the following simplification:
process.getActivityForId(vps.getActivityId()).ifPresent(vps::setActivity);
/*
for (Activity activity : process.getActivities()) {
if (activity.getId().equals(vps.getActivityId())){
vps.setActivityName(activity.getName());
vps.setActivityRealization(activity.getRealization());
break;
}
}
*/
setActivity() is being called as a method reference here.
Create a VisualProcessStep instance with some of step's properties
This part is relatively straightforward, you can encapsulate the logic of setting the properties from a ProcessStep instance to a VisualProcessStep instance as a method inside the latter:
public class VisualProcessStep {
public static VisualProcessStep from(ProcessStep processStep) {
VisualProcessStep vps = new VisualProcessStep();
vps.setActivityId(processStep.getActivityId());
vps.setDefinition(processStep.getDefinition());
vps.setId(processStep.getId());
vps.setParameters(processStep.getParameters());
vps.setStatus(processStep.getStatus());
return vps;
}
}
The code in your original loop can then be just:
VisualProcessStep vps = VisualProcessStep.from(processStep);
/*
VisualProcessStep vps = new VisualProcessStep();
vps.setActivityId(processStep.getActivityId());
vps.setDefinition(processStep.getDefinition());
vps.setId(processStep.getId());
vps.setParameters(processStep.getParameters());
vps.setStatus(processStep.getStatus());
*/
Putting it altogether, #1
After you have extracted out the derivation logic into small methods on your model classes, you should have something similar to the following:
List<VisualProcess> vProcesses = new ArrayList<>();
for (Process process : processList) {
VisualProcess vProcess = new VisualProcess();
vProcess.setId(process.getId());
List<VisualProcessStep> steps = new ArrayList<>();
List<VisualProcessStepConnection> connections = new ArrayList<>();
for(ProcessStep processStep : process.getSteps()){
VisualProcessStep vps = VisualProcessStep.from(processStep);
process.getActivityForId(vps.getActivityId()).ifPresent(vps::setActivity);
steps.add(vps);
connections.addAll(processStep.getConnections());
}
vProcess.setSteps(steps);
vProcess.setConnections(connections);
vProcesses.add(vProcess);
}
From this, the three 'main' steps that you need to do for each step becomes clearer.
Putting it altogether, #2
If you were to take the whole Stream-based approach one step further, you will realize that perhaps, even your looping for the steps can be 'converted' too:
List<VisualProcess> vProcesses = new ArrayList<>();
for (Process process : processList) {
VisualProcess vProcess = new VisualProcess();
vProcess.setId(process.getId());
List<VisualProcessStep> vpsList = process.getSteps().stream()
.map(VisualProcessStep::from)
.peek(v -> process.getActivityForId(v.getActivityId()).ifPresent(v::setActivity))
.collect(Collectors.toList());
vProcess.setSteps(vpsList);
List<VisualProcessStepConnection> connectionsList = process.getSteps().stream()
.map(ProcessStep::getConnections).flatMap(Collection::stream)
.collect(Collectors.toList());
vProcess.setConnections(connectionsList);
vProcesses.add(vProcess);
}
In this case, we are streaming on process.getSteps() twice, once to map it to VisualProcessStep objects, and the other to collect() and flatMap() into the VisualProcessStepConnection objects. This lets us eliminate the temporary steps and connections variables. Somewhat conveniently, we can also perform a peek() on the VisualProcessStep objects to set the activity-related properties before collecting them into a List.
Finally, one can attempt to complete the whole Stream-based metamorphosis on the processList, but I will leave that to the discerning reader seeing as how this is already quite a lengthy answer.
Conclusion
It's always good to conceptualize what are the steps in your chunk(s) of code, and then wrap them into their own methods. This helps to facilitate an appreciation of your own code, which could uncover inefficiencies (e.g. doing something twice) or potential bugs (checking for a property in one place but not the other). More importantly, it lets you easily perform unit testing on smaller slices of your code, so that you can safely modify these smaller chunks in the future to be more efficient with the assurance that you wouldn't be introducing bugs.
