Skip to content

Conversation

@treblereel
Copy link
Collaborator

Many thanks for submitting your Pull Request ❤️!

What this PR does / why we need it:

Special notes for reviewers:

Additional information (if needed):

@treblereel treblereel requested a review from fjtirado as a code owner December 9, 2025 22:03
Comment on lines +55 to +60
for (Activity activity : flexibleProcess.getActivities()) {
TaskExecutorBuilder<?> builder =
factory.getTaskExecutor(position, activity.getTask(), definition);
TaskExecutor<?> executor = builder.build();
executors.put(activity, executor);
}
Copy link
Collaborator

@fjtirado fjtirado Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +64 to +66
} catch (Exception e) {
throw new RuntimeException("Error executing activity: " + activity.getName(), e);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shoulds not catch exception generically

Comment on lines +57 to +59
executor
.apply(workflowContext, parentContext, input)
.join(); // blocking, because we run flexible process one by one
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to run one executor after another, rather that invoking join, we should chain completable future using thenAccept.

Comment on lines +67 to +76
exit = flexibleProcess.getExitCondition().test(input);
if (exit) {
break;
}
}
counter--;
if (counter <= 0) {
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the loop termination condition should be revised, why counter is not part of the while?

Comment on lines +81 to +82
.filter(activity -> activity.getKey().isRepeatable() || !activity.getKey().isExecuted())
.filter(e -> e.getKey().getEntryCondition().test(input))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two filters can be merge into one

return this;
}

public FlexibleProcessBuilder maxAttempts(Integer maxAttempts) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using a int parameter, so you do not have to care about null?

Comment on lines +85 to +88
flexibleProcess.exitCondition =
Objects.requireNonNull(this.exitCondition, "Exit condition must be provided");
flexibleProcess.activities =
Objects.requireNonNull(this.activities, "Activities must be provided");
Copy link
Collaborator

@fjtirado fjtirado Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If exitCondition and activities are mandatory, then, they should be provided when creating the builder rather than using with methods.
Builder pattern should be used when you have few mandatory parameter and a lot of optionals (or defaults)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants