Question: (bad code smell) Is Injector as Singleton bad practice?

[gallardo]

_{(I'm posting here as it seems that the google group https://groups.google.com/g/google-guice/c/sRzQInYrg6Y/m/Wm75m8R5BgAJ has no attention)}

I'm trying to debug an issue (serenity-bdd/serenity-gradle-plugin#9) in a 3rd party framework that uses Guice. The problem has to do with the integration of the framework (serenity-bdd) in a gradle plugin, where some instance isn't recreated when needed.

Inspecting the code, I can summarize what I fell is a bad practice. The code has a Singleton more or less like this (see in the sources: https://github.com/serenity-bdd/serenity-core/blob/82cd173792d9f7898791cab0fe8d3a775bfb2825/serenity-model/src/main/java/net/thucydides/core/guice/Injectors.java):

public class Injectors {
    private static Injector injectors;
    public static synchronized getInjector() {
        if (injector == null) {
            injector = Guice.createInjector (new MyModule());
        }
         return injector;
    }
}

and then uses Injectors.getInjector() to get instances of classes everywhere through the code. For instance (see in the sources: https://github.com/serenity-bdd/serenity-core/blob/82cd173792d9f7898791cab0fe8d3a775bfb2825/serenity-core/src/main/java/net/serenitybdd/core/pages/PageUrls.java#L118-L124):

private static String namedUrlFrom(String annotatedBaseUrl) {
    String pageName = annotatedBaseUrl.substring(NAMED_URL_PREFIX.length());
    EnvironmentVariables environmentVariables = Injectors.getInjector().getInstance(EnvironmentVariables.class);
    return EnvironmentSpecificConfiguration.from(environmentVariables)
        .getOptionalProperty(pageName)
        .orElse(environmentVariables.getProperty(pageName));
}

There are also many places throughout the code where constructors uses the Injector to initialize its fields. For example (see in the sources: https://github.com/serenity-bdd/serenity-core/blob/82cd173792d9f7898791cab0fe8d3a775bfb2825/serenity-core/src/main/java/net/serenitybdd/core/annotations/locators/SmartAnnotations.java#L165-L167):

public SmartAnnotations(Field field, MobilePlatform platform) {
    this(field, platform, WebDriverInjectors.getInjector().getInstance(CustomFindByAnnotationProviderService.class));
}

I have no experience using Guice, and no experience with the framework, so I cannot say if it's very wrong or acceptable. But this (anti?)pattern is giving me hard times trying to debug. As it is now, many functions are quite compact, but I guess we are loosing many benefits of Guice, and I'm not sure if this is a legal use at all.

I have began to rewrite the code and prepare a Pull Request, but it's extremely costly and I'm not sure that I will do better than the current code. So the question for the experts: Is this so bad that it would require a complete rewrite of the relevant code? Am I missing something?

Best regards,

Alberto

[Tembrel]

Not an expert, just a long-time Guice user.

I agree that explicit use of injectors isn't very lovely, not least because it ties dependency injection to Guice rather than some other javax.inject-compliant DI framework. But since serenity-bdd doesn't seem to expose any of its DI stuff to its users, it's only an internal problem.

And it's not a really big problem: From my very cursory scan of the codebase, it looks as though Guice is only being used in a fairly limited way. In contrast, refactoring all that code to do "proper" dependency injection would mean touching a lot of files and introducing a lot of complexity for no compelling reason.

Bottom line: I think the cure would be much worse than the disease, in this case.

[gallardo]

@Tembrel thanks a lot. Your word as long-time Guice user is for me much more reliable than my feelings. I guess it's time to abandon my crusade for the purity of DI.