1

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?

3
  • If app.gallery is so-called an "static asset store", hard-coding it might not be an issue. This kind of usage is very commonly seen in mobile apps and app store apps. The belief is that each app will only have one such static asset manager, and that the content of that static asset manager can be edited in the same software development environment. – rwong Dec 5 '20 at 11:20
  • Can you think of a scenario where it would make sense for an Agent to fetch something from two or more different implementations of app.gallery.getModel within the same version and platform of the software? – rwong Dec 5 '20 at 11:22
  • Its true that app.gallery is a static asset store. What about other access to app inside Agent? What about unit testing? Ok, unit testing graphical aspects may not be feasible but.. still? And to answer your other question: Yes and no, it may be possible in the (far) future, that users could provide own assets - in such a scenario the implementation would have to change. – morpheus05 Dec 5 '20 at 11:37
3

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.

1
  • I agree ideally dependencies should be provided via constructor. I think your idea with the static/instance property is a nice middle way which allows the code to grow and slowly move to pure constructor injection. But hey, for now I have to defend the use of TS so I have to channel my energy. – morpheus05 Dec 5 '20 at 16:17

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.