3
\$\begingroup\$

In my task I have to pass some data from one model to another, where each model contains lists:

List<VisualProcess> vProcesses = new ArrayList<VisualProcess>();
    for (Process process : processList) {
                VisualProcess vProcess = new VisualProcess();
                vProcess.setId(process.getId());
                List<VisualProcessStep> steps = new ArrayList<VisualProcessStep>();
                List<VisualProcessStepConnection> connections = new ArrayList<VisualProcessStepConnection>();
                for(ProcessStep processStep : process.getSteps()){
                    VisualProcessStep vps = new VisualProcessStep();
                    vps.setActivityId(processStep.getActivityId());
                    vps.setDefinition(processStep.getDefinition());
                    vps.setId(processStep.getId());
                    vps.setParameters(processStep.getParameters());
                    vps.setStatus(processStep.getStatus());
                    for (Activity activity : process.getActivities()) {
                        if (activity.getId().equals(vps.getActivityId())){
                            vps.setActivityName(activity.getName());
                            vps.setActivityRealization(activity.getRealization());  
                            break;
                        }
                    }
                    steps.add(vps);
                    for (ProcessStep nextProcessStep : processStep.getNextSteps()) {
                        VisualProcessStepConnection vpsc = new VisualProcessStepConnection();
                        vpsc.setFrom(processStep.getId());
                        vpsc.setTo(nextProcessStep.getId());
                        connections.add(vpsc);
                    }
                }
                vProcess.setSteps(steps);
                vProcess.setConnections(connections);
                vProcesses.add(vProcess);
            }

However, while I think it looks awful, I don't have any other ideas as to making it more elegant. Another issue is about the size of the method: Would it be better to split it into several, smaller methods?

\$\endgroup\$
  • \$\begingroup\$ Be sure, you'll get an answer, but in the meantime please improve you title (you can edit your question anytime; just avoid changes which would invalidate already given answers). \$\endgroup\$ – maaartinus Jun 28 '15 at 20:25
  • \$\begingroup\$ Are you on Java 6/7/8? \$\endgroup\$ – h.j.k. Jun 29 '15 at 7:26
  • \$\begingroup\$ i'm using java 7 \$\endgroup\$ – viavad Jun 30 '15 at 10:59
3
\$\begingroup\$

Is it too awful code?

Not too awful and not even bad, but the huge wall of code looks ugly.

There are a few minor issues like missing blank after "for" in

for(ProcessStep processStep : process.getSteps()){

and before "{" in

if (activity.getId().equals(vps.getActivityId())){

but that's something your IDE can do for you (Ctrl-Shift-F in Eclipse).


Otherwise, all you need is to extract a few methods like

private VisualProcessStep toVisualProcessStep(Process process, ProcessStep processStep) {
    VisualProcessStep result = new VisualProcessStep();
    result.setActivityId(processStep.getActivityId());
    result.setDefinition(processStep.getDefinition());
    result.setId(processStep.getId());
    result.setParameters(processStep.getParameters());
    result.setStatus(processStep.getStatus());
    for (Activity activity : process.getActivities()) {
        if (activity.getId().equals(result.getActivityId())) {
            result.setActivityName(activity.getName());
            result.setActivityRealization(activity.getRealization());  
            break;
        }
    }
    return result;
}

This can be repeated many times and you surely shouldn't extract everything. When the methods don't get used elsewhere, this is very subjective as you're trading a too long method for too many methods.

The sweet spot are methods somewhere about 15 lines long. As all the methods do is just copying everything and there isn't much to think about, they may be a bit longer. I'd personally split it into probably 3 methods.


Obviously, there may be a smarter way avoiding the manual copying. It surely can be done via reflection, but reflection is verbose and slow and ugly itself, so I wouldn't consider it for this task.

Maybe you could create a constructor (or a factory method) for VisualProcessStep?

\$\endgroup\$
  • \$\begingroup\$ thank you, it's the most perfect answer which i ever waited \$\endgroup\$ – viavad Jun 28 '15 at 20:49
  • 1
    \$\begingroup\$ The suggested method is just missing the process argument. :) \$\endgroup\$ – h.j.k. Jun 29 '15 at 7:27
3
\$\begingroup\$

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.

\$\endgroup\$

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.