Concise and exception-prone method for running method on component

Question

I have a list of Components. When an actor is connected, I want to run onActorConnected on each component. However, if a component's onActorConnected method throws an exception for whatever reason, this cannot affect other components' execution of that method.

Here's a minimized extract of my code:

import static java.lang.String.format;
public class Match {
 private final Set<Actor> actors = ...;
 private final List<MatchComponent> components = ...;
 public void connectActor(@NotNull Actor actor) {
 if (actors.contains(actor))
 throw new IllegalArgumentException(format("Actor '%s' is already connected.", actor.getName()));
 actors.add(actor);
 // This is hard to read!
 // But I will be doing this in a lot of places, so I want to keep in short.
 components.forEach(c-> runOrLogException(() -> c.onActorConnect(actor), c.getName(), "onActorConnect"));
 }
 // The same pattern will be used in `disconnectActor` and other similar methods.
 private void runOrLogException(@NotNull Runnable runnable, @NotNull String instanceName, @NotNull String methodName) {
 try {
 runnable.run();
 } catch (Exception e) {
 logger.log(Level.SEVERE, format("'%s' on '%s' in '%s' threw an exception.", methodName, instanceName, this.getName()), e);
 }
 }
}

My question primarily is about whether this is a good way to do it? I fear I might be missing some way of using Java's lambdas to make this cleaner.

Bonus question is whether it's considered good practice to throw an exception if an "actor" is already connected. I considered just returning false if this is the case, but I do not ever expect myself to call this method if an actor is already connected. If that happens, something has gone wrong somewhere. I instead provide an isActorConnected method that the caller can use to explicitly handle such a case.

• Bobby: "But I will be doing this in a lot of places, so I want to keep in short." as in "it must run fast" or as in "copy and paste"?
• Bobby: Maybe I'm failing to see something here, but couldn't you do it with runOrLogException(Actor actor, Function<Actor> function) and then call it with runOrLogException(actor, actor::onActorConnected)? That would clear the calling code up.

Answer 1

Are there certain exceptions you can anticipate for onActorConnected? Let's make an encompassing checked exception ActorUnconnectableException, though this would need to be be made more finegrained for particular connectivity issues:

class ActorUnconnectableException extends Exception {
 ActorUnconnectableException(Actor actor, MatchComponent component) {
 super(
 String.format("actor: %s cannot connect to component: %s", actor.getName(), component.getName())
 );
 }
}

And declare onActorConnect within MatchComponent like this:

void onActorConnect(Actor actor) throws ActorUnconnectableException {
 ...
}

The reason I advocate for a checked exception here is because it sounds like the it is hard to guarantee that an Actor is connectable without attempting to connect it. If we were able to verify that an Actor is connectable beforehand, an unchecked exception would be better suited here.

For something like a string that we're trying to parse as a number, let's say this parsable string is typed in our frontend. We can enforce through whatever frontend framework that this string contains only digits. That's why Integer::parseInt throws an unchecked exception, it expects that the validation is done before hand.

Another thing that can be improved is this:

if (actors.contains(actor))
 throw new IllegalArgumentException(format("Actor '%s' is already connected.", actor.getName()));
actors.add(actor);

Apart from adding braces for the if, we can utilize the fact that actors::add returns true if actor is not contained, otherwise false, like this:

boolean actorAdded = actors.add(actor);
if (!actorAdded) {
 throw new IllegalArgumentException(format("Actor '%s' is already connected.", actor.getName()));
}

This should remain an unchecked exception cause the calling code should be able to determine whether it already added an Actor equal to actor beforehand.

The rest of connectActor ends up looking like this:

var unconnectableComponents = new ArrayList<MatchComponent>();
for (MatchComponent component : components) {
 try {
 component.onActorConnect(actor);
 }
 catch (ActorUnconnectableException e) {
 unconnectableComponents.add(component);
 }
}
// Log `unconnectableComponents` or create new exception to report their collective failure.

This definitely ends up looking verbose, but the idea is that we want things outside of the calling code's control (like whether can actor is in fact connectable, which sounds like it can't be decided beforehand) to be checked at compile time.