0

To set the stage, I am trying to do pure dependency injection for a Java Library I am creating to make it more testable. As it is a library, I want to do pure dependency injection without creating a DI container/composition root as discussed by Mark Seemann in his blog posts*. The problem I am running into is that I have some visitor classes that must contain state (an example illustrating the concept is below). Would it be cleaner to inject these or just declare them using the new keyword? The only problem I see with injecting them is that I would need a function to clear the state of the visitor after every call vs. using new I would not have to. Is there any best practice for this?

public class CarServiceBuilder {

    public CarService buildCarService(){      
      CarPartPrinter carPartPrinter = new CatPartPrinter();
      CarService carService = new CarService(carPartPrinter);
      return carService;
    }

}

public class CarService {

    private final CarPartPrinter printParts;
    
    public CarService(CarPartPrinter carPartPrinter){
    this.carPartPrinter = carPartPrinter;
    }
    
    public void printParts(Car car){
        CarVisitor carVisitor = new CarVisitor();
        car.accept(carVisitor);
        List<Part> parts = carVisitor.getParts();
        carPartPrinter.printParts(parts);
    }
    
}


public class CarVisitor extends Visitor {

   private List<Parts> parts;

   public CarVisitor(){
    parts = new ArrayList<>();
   }

   @Override
   public void visit(Part part){
    parts.add(part);
   }
   
}

VS.

public class CarServiceBuilder {

   public CarService buildCarService(){ 
      CarVisitor carVisitor = new CarVisitor();
      CarPartPrinter carPartPrinter = new CatPartPrinter();
      CarService carService = new CarService(carVisitor, carPartPrinter);
      return carService;
   }

}

public class CarService {

    private final CarVisitor carVisitor;
    private final CarPartPrinter printParts;
    
    public CarService(CarVisitor carVisitor, CarPartPrinter carPartPrinter){
    this.carVisitor = carVisitor;
    this.carPartPrinter = carPartPrinter;
    }
    
    public void printParts(Car car){
        car.accept(carVisitor);
        List<Part> parts = carVisitor.getParts();
        carPartPrinter.printParts(parts);
        carVisitor.clearState();
    }
    
}


public class CarVisitor extends Visitor {

   private List<Parts> parts;

   public CarVisitor(){
    parts = new ArrayList<>();
   }

   @Override
   public void visit(Part part){
    parts.add(part);
   }

   public void clearState(){
    parts = new ArrayList<>();
   }

}

Related blog posts:

3 Answers 3

2

I don't typically think of visitors as a dependency that you would inject. Visitors are just a control flow mechanism, an object-oriented switch statement of sorts. Any state within the visitor is analogous to local variables you might otherwise define in the caller's scope. You also usually know exactly what type of visitor is required in a given context (as is the case in your example: printParts knows it needs a CarVisitor to collect a list of parts), so it is simplest to just create a new local instance and throw it away once you're done. Injecting stateful objects creates issues like the one you already noted: you need to remember to clear state once you're done, and it's not thread-safe.

As for testing, I would just treat the visitor as an implementation detail of CarService and test the functional requirements directly, i.e. does printParts actually print the correct parts, regardless of how it is implemented.

5
  • I've written and deleted an answer to this question two times already. Something about the design seemed a little off, and your answer sums it up perfectly: the visitor should not maintain state. The OP is trying to solve the wrong problem. Aug 31 at 22:27
  • @GregBurghardt I agree w/ casablanca but I disagree that a visitor should not maintain state. It is not that simple and is very much domain dependent. For example, if you ever build a parser with ANTLR or lex/yacc, and parse a language, you need to walk that parse tree and gather data from specific nodes. The only way to do this is to maintain state in that visitor. Depending on the domain, maintaining state in a visitor, while not 'ideal', is very common. Aug 31 at 22:32
  • @EspressoCoder: That's fair. I might still (re)(re)post my answer. Aug 31 at 22:35
  • @EspressoCoder: I undeleted it just as you posted a comment. Aug 31 at 22:36
  • @Casablanca Thank you for this answer. What cinched it was the way you described visitor as really nothing more than a control flow (like a switch) vs. a business logic class. Aug 31 at 23:33
0

The composition root is where long lived objects are constructed. Build em in main and they die when main dies. But not everything lives that long or is born that early.

These other, short lived objects, are known as Ephemeral objects. I mean, if you build everything in main where are you supposed to build a timestamp? No the rule is to build objects as high up the call stack as you can. But no higher.

So if you really need CarVistor objects to be born late and die early, then let them.

What you may wish to consider is how comfortable you are with the static dependency of CarService on knowing exactly what a CarVisitor is concretely. Now if they're both distributed in the same jar (or whatever) this may be no big deal. If not, if they need to live independent lives, and if CarVisitor can't be trusted to be stable (which is why String is immune to this consideration), then you might want an injectable abstract factory that produces a new CarVisitorLikeThingy whenever you ask it. Push how that's configured as high up the call stack as you can and CarService can live in blissful ignorance of CarVisitor.

Just be sure you really need that though. Because doing this with every little thing gets annoying quickly.

0

There are no best practices for this, just requirements, and trade-offs.

There is nothing wrong with passing the visitor as a constructor argument. As you noted, the CarVisitor would need a public method to clean up its internal state. Also nothing wrong with that. Public methods define a contract between your library and consumers. Requiring the visitor in the constructor for CarService has a few implications you should be aware of, though:

  • To facilitate mocking and unit testing, CarVisitor should be an abstract class or interface.
    • This implies consumers can provide their own implementations in production code, too, not just in tests.
  • Consumers of CarService must initialize the visitor prior to initializing the service.
  • Consumers will need to initialize one service for each visitor, which depending on the requirements your library is trying to satisfy, might not be desirable.

You increase complexity in order to make testing easier. This is a trade-off with this design, and in many cases like this where you want to mock dependencies.

The other variation where the visitor gets initialized in the printParts method removes the need for a "cleanup" method at the expense of making this class harder to test. The CarService is tightly coupled to the CarVisitor, leaving no room for specialization in the future — in your library or from consumers. At the same time you take control of cleaning up the internal state of a visitor, you take control away from your test code. This is another trade-off.

An additional trade-off that isn't immediately apparent when a class initializes it's own dependencies is that the CarService must also know the dependencies of its dependencies. The printParts method must have access to all other objects that must be passed to the CarVisitor constructor. Often, this means CarService will require additional constructor parameters just so it can initialize a visitor, and not because the service needs those parameters directly. Any change to the initialization of a visitor requires a change to the CarService constructor as well. This is one reason dependency injection ultimately makes things easier.

While an extra "cleanup" method for the visitor seems messy, it is much easier to understand than requiring CarService to juggle the dependencies of its dependencies when initializing the visitor in the service class. I recommend passing the visitor as a constructor argument.

2
  • I appreciate this answer, it's very thorough. I will upvote it as soon as I get enough reputation."To facilitate mocking and unit testing, CarVisitor should be an abstract class or interface". I agree with this in theory and I know it is what uncle bob pitches. The only thing I disagree with is "when" to do this is practice. I stick to the methodology of only declare interfaces when they are needed. Have been using spring for 10+ years and I have found that creating them every time you want to inject a dependency will cause serious bloat in the code. Like you say, everything is a tradeoff. :) Aug 31 at 22:51
  • Yup. And the trade-off is your tests need to initialize a real CarVisitor. If it's needs are simple, no big deal. Aug 31 at 23:43

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.