0

I have a class that I usually end up calling XManager. This class usually acts a dispatcher. Where it will read a config and do what the config says.

Assuming that I have the following class structure:

-Manger
 | - Phases
   | - PhaseX implements Phase { void run(); }
   | - PhaseY implements Phase { void run(); }
   | - PhaseZ implements Phase { void run(); }
 PhaseManager

In PhaseManager I have some code that looks like:

class PhaseManager {
    private Set<Phase> phases;

    public PhaseManager(Config config) {
        // Use config to instantiate all enabled phases, adding them to phases.
    }

    public void runPhases() {
        for(Phase p : phases) {
            p.run();
        }
    }
}

And thus in my main.java I'd have something like:

PhaseManager manager = new PhaseManager(config);
manager.runPhases();

But I'm convinced there is a better way. And after reading many of the design patterns I cannot figure out exactly to which design pattern this best fits.

It looks like it could be callback, but not really; as there is no action that occurs to initiate this, it just has to happen.

It could be command, but it seems like command is a bit overkill for this approach.

  • Looks like a simples class to me. If you want to be really fancy, call it a Façade. By the way, I'm not a big fan of "Manager" classes. What is it trying to manage ? Which process ? "Manager", "Runner", "Executer", "Doer" don't mean anything. – Machado Jul 28 '17 at 19:36
  • These are the reasons I posted here. I want to write better code, and Jeff Atwood's adage holds true. I've implemented the facade design pattern and I like this much better. What would be a better naming convention for "manager"? PhaseFacade? I agree completely that those words have no meaning; but I know not a better vernacular. – lilott8 Jul 28 '17 at 21:00
  • What do your phases do when they run? My gut is telling me that your phase.run() might fit into a Java Callable, and then all your phases go into a Java ThreadPoolExecutor. What is your "manager" class supposed to manage? – Thomas Carlisle Jul 28 '17 at 21:05
  • It run difference phases of a compiler. So it might run an optimization, it might run type inference, etc. – lilott8 Jul 28 '17 at 21:08
  • "It could be command, but it seems like command is a bit overkill for this approach." How could it be an overkill when you are just looking for a name of an existing pattern that describes what you already have. A name cannot be an overkill. It would be overkill if you where looking for a pattern to implement, since "Command" is more complicated than what you need. – Tulains Córdova Jul 29 '17 at 4:50
1

I think the same can (and likely should) be expressed cleaner using the builder pattern.

Result result = InitialPhase.fromConfig(config)
                            .phase1(...)
                            .phase2(...)
                            // ...
                            .build();

Each .phaseN() method may return a class that only allows to invoke the correct next phase(s). (In the "PhaseManager" design it is possible to run the phases in arbitrary order.) Each .phaseN(...) may accept whatever phase-specific parameters there may be.

The final .build() makes the resulting object from a phase object. If the subject area permits, several phases can provide a .build() method, allowing for different build paths.

  • The builder pattern is great for creating complex (immutable) structures. Since the result should be a list of phases (same base type) that are enabled, I don't see how the builder pattern applies nor how does it help to make it clear that you are taking a list of phases and filtering the disabled ones based on a config. – Zdeněk Jelínek Jul 29 '17 at 10:40
  • @ZdeněkJelínek: I mostly suppose that for(Phase p : phases) {p.run();} is likely not the optimal design choice, hence the type-safe loop unrolling. This depends greatly on the subject area, though. – 9000 Jul 29 '17 at 19:47
  • From what I gather, you assume that the phases will have some dependencies between themselves and can only be ran in a specific order in the final production code. In that case, your answer makes sense. But I'm afraid that the question is more general than that. – Zdeněk Jelínek Jul 30 '17 at 7:48
  • Correct, in my case, the phases aren't necessarily dependent. So this works quite well. – lilott8 May 10 '18 at 18:56
3

I haven't enough reputation to comment, so let's go for an answer.

Your class violates the SRP because it : 1. uses config to instantiate all enabled phases, 2. run the phases.

I would prefer something like:

class PhasesCreator {
    List<Phase> createPhases(Config config) { ... }
}

In your main, run your phases like this:

for(Phase p : phases) {
        p.run();
}

Or with the Java 8 style:

phases.forEach(Phase::run);

You may also wrap the run of the phases in a dedicated class, for example a PhasesRunner class:

class PhasesRunner {
     void run(Iterable<Phase> phases) { ... }
}

EDIT To summarize:

  • You need a creational pattern (factory, abstract factory, builder, ...) to instantiate the phases collection ;
  • You may or may not need a specific pattern (e.g. command) to run the phases, but I don't have enough information to give an opinion.

Aside from the original question:

It seems that you are writing a compiler. I don't see the flow of data from text to binary (characters, tokens, ...) in the current design. You should, at least, enforce the order of the phases using an input type an a return type for the run method:

interface Phase<I, O> {
    O run(I input);
}

But running the phases won't just be a loop anymore.

  • Yes. My apologies for the poor comment. It doesn't actually parse the config, it just uses it. I edited my question to reflect that. – lilott8 Jul 28 '17 at 21:02
  • @lilott8 I edited my answer: you don't have a config parser, but a config reader that "Use config to instantiate all enabled phases, adding them to phases." That's one responsibility, running is the other responsibility. – jferard Jul 28 '17 at 21:09
  • "Config reader" was a poor word choice: it's a "phases creator" from the config data. – jferard Jul 28 '17 at 22:22
  • I like the thinking behind this answer but I don't feel it really helps with the naming issues of this question. I wouldn't call "A component that creates objects and filters them based on an input" a Creator. I propose to create two components to adhere to the business rules (1. There is a given list of existing phases; 2. These phases are filtered in runtime based on what is enabled in the config). This would improve OCP adherence and give you Phases or PhasesFactory and ConfigPhasesFilter. Another option is a Phase::IsEnabled(Config) method but that couples Phase with Config. – Zdeněk Jelínek Jul 29 '17 at 11:12

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.