Dear List, I'm trying to debug an issue (https://github.com/serenity-bdd/serenity-gradle-plugin/issues/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 -- You received this message because you are subscribed to the Google Groups "google-guice" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/google-guice/cad1979e-375c-48ae-837d-77fc466b2dd8n%40googlegroups.com.
