Resolve concrete object in constructor from a given id

Question

I have the following class

export class Agent {
  constructor(modelName: string) {
    this.shape = app.gallery.getModel(modelName);
    
      ...
  }
   
  randomMethod() {
    ... // using app for other stuff....
  }
}

Which I find is very bad software design. My reasoning is as follows:

• Access of a static var inside an object (constructor) - it should be passed in. Why bad:
  • Dependent of the existence of "app" and that "app" has an object gallery
  • As such this code cannot be tested
• Instead of passing in a string, you could pass in the finished object (model in this case)
• Global var "app".

My problem is, that it seems, I'm alone with this opinion inside the team. Other members stated that they want to construct an instance of agent simply by:

const agent = new Agent("chicken");

Insteaed of using Factory/Factory Method or something similar.

My first simple suggestion was this:

new Agent(app.gallery.getModel("chicken"), app);

Since the class Agent uses app through its code, I would pass it in, instead of accessing it via global var. This first suggestion was denied by the team stating that:

• The API is to complex (see constructor string example above).
• Tied deadline, cannot discuss this minor details

Given the context, I have the following questions:

• Is my reasoning for disliking this kind of code style (accessing a static inside constructor, using a static throughout the class instead of using a field) correct?

• What alternative are there to simplifying the constructor process?

• If my intuition is right, what kind of paper/argument can I bring forth?

Answer 1

The app object is a dependency of Agent. Following the guidelines for dependency injection, the app object should be a parameter in the constructor for Agent. The modelName is another dependency, so it too should be passed to the constructor.

A simple compromise would be to pass both:

var agent = new Agent(app, "chicken");

This gets rid of the global variable, yet object construction is still pretty simple.

An alternative would be to create a factory method in app to create Agent objects, which gives developers a method with a single argument:

var agent = app.createAgent("chicken");

This becomes awkward over time, because it sounds like this app object is a pretty common thing. It could become 90% factory methods with 10% logic. When this happens, you've basically created a service locator, which can easily become an anti-pattern.

Yet another compromise could be using a static property on Agent, and an instance property on the same class. Initialize the static property when the application is started, and simply return the static property from an instance property. This gives the illusion of dependency injection.

class Agent {
    static app;
    
    get app() {
        return Agent.app;
    }
    
    get gallery() {
        return this.app.gallery;
    }
    
    constructor(modelName : string) {
         this.shape = this.gallery.getModel(modelName);
    }
    
    someMethod() {
        this.app.foo();
    }
}

// In area where application is initialized:

var app = ...

Agent.app = app;

This way you can add the extra constructor parameters later and only need to refactor object initialization.

This can assist unit testing as well, because you can assign a mock app to the static property:

Agent.app = mockApp;

// Rest of unit test goes here

Ideally, I would say to add the app as a constructor parameter. It is only one extra parameter, so object initialization is still pretty simple. If that is really not acceptable, then try for the compromise solution using static and instance properties, so instance methods can be written as if proper dependency injection is used.