3
\$\begingroup\$

In my project I have the following base interfaces used through the application:

interface Node { /* ... */ }
interface NodeService<T> { /* ... */ }

Concrete classes might look as follows:

class User implements Node { /* ... */ }
class UserService implements NodeService<User> { /* ... */ }

class Entity implements Node { /* ... */ }
class EntityService implements NodeService<Entity> { /* ... */ }

In the end I want to have a Map<String, Class<? extends NodeService<Node>>> that in the case above might look as follows:

Map{
    User=UserService.class,
    Entity=EntityService.class
}

My current solution has the following approach:

  1. Using the Reflections library find all classes implementing NodeService
  2. Iterate over all generic interfaces of said classes until we find the NodeService interface
  3. Access its first and single generic type argument
  4. Cast the type to a class and map it to its simple name
  5. Build a tuple (using vavr in this case) containing the simple name and the service class we found in step 1
  6. Flatten the result and collect the tuples into a map
@SuppressWarnings({"unchecked", "rawtypes"})
final Map<String, Class<? extends NodeService<Node>>> nodeServices =
    new Reflections(BackendApplication.class.getPackageName())
        .getSubTypesOf(NodeService.class).stream()
            .map(
                serviceClass ->
                    Stream.of(serviceClass.getGenericInterfaces())
                        .filter(ParameterizedType.class::isInstance)
                        .map(ParameterizedType.class::cast)
                        .filter(type -> type.getRawType().equals(NodeService.class))
                        .map(ParameterizedType::getActualTypeArguments)
                        .map(typeArguments -> typeArguments[0])
                        .filter(Class.class::isInstance)
                        .map(Class.class::cast)
                        .map(Class::getSimpleName)
                        .map(nodeClass -> Tuple.of(nodeClass, (Class) serviceClass))
                        .collect(Collectors.toSet()))
            .flatMap(Collection::stream)
            .collect(Collectors.toMap(Tuple2::_1, Tuple2::_2));

I am surprised that this works in the first place but the code is ridiculously complex and I am looking for suggestions on how to rewrite this. Some of my biggest pain points are the handling of the classes generic interfaces even though in the end I'm only interested in a single one of them making the stream in the middle introduce unnecessary nesting. Furthermore the way I attempt to navigate to the correct type is really bothering me as well.

Thus I am open for your input on how to improve this chunk of code.

The performance of this snippet is surprisingly good. I am mainly looking for input on the readability and refactoring this to make it much more maintainable, even at the cost of performance since this will be only invoked once during the application lifecycle.

\$\endgroup\$
2
\$\begingroup\$

Should we expect that there are services that have a different type parameter? Because otherwise just a Set of services may suffice, or you can simply remove the Service part of the name during the mapping. This is an obvious thing from the example, but I'm not sure how far the example matches your actual code.

What I often see when streams are used is that suddenly the split up into methods is forsaken. For instance, in my code I would first build a list of services and then create a map out of it. I don't think that an intermediate list would do much w.r.t. performance here (the number of services will be limited, loading the classes will take much more time, and the streaming will have intermediate representations as well), and the code would clearly be simplified. Method chaining can be nice, but please don't overdo it.

Similarly, a method that tests if a class implements an interface can be clearly split out:

Stream.of(serviceClass.getGenericInterfaces())
    .filter(ParameterizedType.class::isInstance)
    .map(ParameterizedType.class::cast)
    .filter(type -> type.getRawType().equals(NodeService.class))

And actually, if you do that and look for code you will find NodeService.class.isAssignableFrom(clazz). The part is well programmed but please do use library functions when they are available.

All of above may not do much with regards to performance, but taking it into account should definitely help with the readability. Currently you had to describe what the code does in the question description. That should not be necessary for well programmed / documented code.

\$\endgroup\$
5
  • \$\begingroup\$ With regards to your first paragraph: each services that handles any type T exposes itself to the map by implementing NodeService<T> and will always be responsible for the type T. Dropping the Service part of the name was something I thought about as well but it would enforce a particular naming scheme of those exposed services. Something that I wanted to not enfore in the first place. \$\endgroup\$ – Christian Ivicevic May 4 '20 at 13:14
  • \$\begingroup\$ I guess you can reason either way, but in the end I guess that one is subjective. It would also be slightly brittle when it comes to spelling things wrong, but that should show up during testing for normal code. I had a similar command interface and registry but it was easier as I didn't rely on type parameters and used an abstract class instead. Do note that I had some issues with it: during debugging any change resulted in hot code replace failing, so I had to make a manual map in the end. \$\endgroup\$ – Maarten Bodewes May 4 '20 at 13:22
  • \$\begingroup\$ Toned my answer down a bit. That was too negative - maybe because I don't like programming with streams much. It hurts readability too much in my opinion. \$\endgroup\$ – Maarten Bodewes May 4 '20 at 23:48
  • \$\begingroup\$ Don't worry, I was mainly waiting for further input by others until I eventually came up with an entirely different solution similar to you approach from the comment. \$\endgroup\$ – Christian Ivicevic May 5 '20 at 19:07
  • \$\begingroup\$ Cool, glad to have been of help. \$\endgroup\$ – Maarten Bodewes May 5 '20 at 19:15
0
\$\begingroup\$

Today I was able to simplify my approach by introducing a default method into the NodeService interface and leverage Springs dependency injection for lists. The interface looks as follows:

public interface NodeService<T extends Node> {
  @SuppressWarnings("rawtypes")
  default String getNodeType() {
    return ((Class)
            ((ParameterizedType) getClass().getGenericInterfaces()[0]).getActualTypeArguments()[0])
        .getSimpleName();
  }

  /* ... */
}

The respective class using this method to find the respective service ended up like this:

@RequiredArgsConstructor
public class Query implements GraphQLQueryResolver {
  private final List<NodeService<?>> nodeServices;

  public Node node(final String id) {
    try {
      val nodeId = NodeId.fromString(id);
      return nodeServices.stream()
          .filter(service -> service.getNodeType().equals(nodeId.getType()))
          ./* ... */;
    } catch (final IllegalArgumentException e) {
      /* ... */
    }
  }

  /* ... */
}

Of course I haven't mapped the services to a map as initially wanted, but this would be trivial with the newly introduced method.

\$\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.