2
\$\begingroup\$

I have a service class that handles recipe management using a piquetteClient (piquette is a web app with a rest API that can run puppet scripts).

The thing is this service class has a bunch of methods like runRecipe, searchRecipes, filter, list etc, and they all require a piquetteClient instance. Because I wanted to not be able to run any of this methods without having first a valid instance of piquetteClient and because I don't want to have it as a parameter for each method (and don't also I don't like to have it externally with a setter because then you give the user the option to just not call that setter - it's confusing), I create a public inner class Runner inside this service and buildRunner method outside the inner class that can give you an instance of the runner only after you provide a valid piquette client instance, after that, the same instance will be used everywhere.

What do you think?

@Component
public class RecipeServiceImpl implements RecipeService {

    private static final long serialVersionUID = 1000050450505050443L;

    @Override
    public Runner buildInternalRunner(PiquetteClient piquetteClient) {
        // validate client instance
        Runner runner = new Runner(piquetteClient);
        return runner;
    }

    public class Runner {

        private final PiquetteClient piquetteClient;

        Runner(PiquetteClient piquetteClient) {
            this.piquetteClient = piquetteClient;
        }

        public List<Recipe> searchRecipes(String query, int limit, int offet) {
            return filterRecipes(piquetteClient.recipes(query, Integer.MAX_VALUE, 0), query, limit);
        }

        /**
         *
         * @param recipeName
         * @param asset
         * @return Future of Integers
         */
        @Async
        public Future<Integer> runRecipe(String recipeName, String asset) {
            // trying to get the correct asset
            String regexAsset = RegexUtil.ANY_CHAR + asset + RegexUtil.ANY_CHAR;
            List<Artifact> artifacts = listArtifacts(recipeName, regexAsset);
            if (!artifacts.isEmpty()) {
                if (artifacts.size() > 1) {
                    log.warn("Multiple artifacts matched... choosing the first one: " + artifacts.get(0).getBuild());
                }
                URI uri = artifacts.get(0).getBuild();
                log.info("Running: " + artifacts.get(0).getBuild());
                return new AsyncResult<>(piquetteClient.runRecipe(recipeName, uri).getId());
            } else {
                log.error("Nothing for: " + recipeName + " and asset: " + regexAsset);
            }
            return new AsyncResult<>(NumberUtils.INTEGER_ZERO);
        }

        // other similar methods 
}

RecipeService interface:

public interface RecipeService extends Serializable {
     RecipeServiceImpl.Runner buildInternalRunner(PiquetteClient piquetteClient);
}

I can't use the constructor because RecipeServiceImpl is a Spring component. The build method it's an override because it's declared in the Service interface so I can inject the service everywhere and use the method. Maybe the name Runner is a bad one, but I was asking how this concept feels like; you have a service implementation that has another object inside that does the actual job. The service itself is just validating and delegating... From outside you can't have a reference to the runner object without having first an instance of the service itself and provide to this instance the necessary data.

\$\endgroup\$
3
\$\begingroup\$

I'm a bit confused by your code -- RecipeServiceImpl.buildInternalRunner has a return type of RecipeServiceImpl.Runner, and it's an Override? Really?

Runner seems to be badly named, as it is easily confused with Runnable, which means something very different. Is this really a RecipeServiceClient?

It's additionally confusing because you seem to be suggesting that RecipeService declares searchRecipes, but your runner doesn't implement that interface....

Given what you have shown thus far, I would have expected to see a single class, call it PiquetteRecipeService, that extends RecipeService but otherwise looks like the Runner you implemented.

class PiquetteRecipeService implements RecipeService {
    private final PiquetteClient piquetteClient;

    PiquetteRecipeService(PiquetteClient piquetteClient) {
        this.piquetteClient = piquetteClient;
    }

    public List<Recipe> searchRecipes(String query, int limit, int offset) {
        ...
    }

    // And so on....
}

This implementation does require that a PiquetteClient be created before the Service is created, but if the service can't do anything without the client, then that's the Right Thing[tm].

If that's not the right answer, then I think you haven't provided a clear description of the constraints you are trying to satisfy.

public interface RecipeService extends Serializable {
    RecipeServiceImpl.Runner buildInternalRunner(PiquetteClient piquetteClient);
}

OK, this is beyond broken. You've got an interface, that declares a function which returns a type defined by a specific implementation of that interface. Help, help, I'm being recursed!

BUT...

The good news is that you are clearly groping toward a Factory pattern. If we change the spelling of a few things...

public interface RecipeServiceFactory {
    RecipeService createService(PiquetteClient piquetteClient);
}

In other words, instead of expecting Spring to create the service for you, you tell it to create an instance of the service factory for you, and then use that factory to create instances of the service when necessary.

public class DefaultRecipeServiceFactory {
    RecipeService createService(PiquetteClient piquetteClient) {
        return new PiquetteRecipeService(piquetteClient);
    }
}

Meanwhile, you would have a new interface declaration, using the old name...

public interface RecipeService {
    List<Recipe> searchRecipes(String query, int limit, int offet);
    Future<Integer> runRecipe(String recipeName, String asset);
    // ...
}

and the PiquetteRecipeService would implement this interface, delegating calls to the client.

Going back to your original question; with this design, yes it does it make sense to combine the service implementation and the factory that is coupled to it in a single class file. I'd be more likely to nest the factory inside the service implementation, rather than the other way around:

public class PiquetteRecipeService implements RecipeService {
    public static class Factory implements RecipeServiceFactory {
        // ...
    }
}

If the service implementation isn't supposed to be public, then I would nest them the other direction (or keep them completely separate).

I still wouldn't quite be happy with this design, because arranging for the Factory to accept a PiquetteClient argument rather gives away the fact that the you are using Piquette under the covers.

Spring basically is a giant factory, so why do we need to roll our own? If you were to let Spring create the PiquetteClient, for instance, then it could inject that client into the RecipeService it creates too. If the problem there is that you need credentials, or a configuration or something to create the PiquetteClient, then maybe that's the argument that should be passed to the Factory. Unclear -- just noting that as it stands, it is a bit odd.

\$\endgroup\$
  • \$\begingroup\$ Yes, sorry, I can't use the constructor because RecipeServiceImpl is a Spring component. The build method it's an override because it's declared in the Service interface so I can inject the service everywhere and use the method. Also I don't understand why would Runner implement the interface... maybe the name Runner is a bad one, but I was asking how this concept feels like; you have a service implementation that has another object inside that does the actual job. The service itself is just validating and delegating... \$\endgroup\$ – spauny Aug 6 '14 at 19:16
  • \$\begingroup\$ I think it would help make things clear if you were to include RecipeService in your question. \$\endgroup\$ – VoiceOfUnreason Aug 6 '14 at 20:23
  • \$\begingroup\$ I added RecipeService \$\endgroup\$ – spauny Aug 6 '14 at 20:48
  • 1
    \$\begingroup\$ +1 for "You've got an interface, that declares a function which returns a type defined by a specific implementation of that interface. Help, help, I'm being recursed!" That's really weird code smell, that an interface's method signature 'know' about the implementation. \$\endgroup\$ – h.j.k. Aug 7 '14 at 3:47

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.