2
\$\begingroup\$

I've implemented a class, which provides access to different static resources, which are used by different frontends. I'm hesitating, because in Java getters and setters are quite wide spread. So my question is, if this code smells bad?

public class ResourceManager {
    public static final Preferences Preferences = new Preferences();
    public static final ResourceBundle Texts = ResourceBundle.getBundle("com.stackexchange.resources.Language");

    public static Image getImage(String name) {
        Image retVal = null;

        try {
            retVal = ImageIO.read(ClassLoader.getSystemResource(name));
        } catch (IOException e) {
            // ToDo Use logging functionality
        }

        return retVal;
    }

    private ResourceManager() {
    }

    public static class UserInterface {
        public static final Font titleFont = new Font("default", Font.PLAIN, 18);
    }
}
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

The fact that you have "Manager" as part of the name of the class is a code smell. This class violates the Single Responsibility Principle.

In particular, Preferences sounds like it contains user-specific data; I wouldn't call it a "resource". It should be split into its own class.

On the other hand, Texts, getImage(), and UserInterface.titleFont appear to contain data that would be application-wide constants. A class like this could make sense:

public class UIResources {
    public static UIResources getInstance(Locale locale) {
       …
    }

    public Image getImage(String name) {
        …
    }

    public String getText(String name) {
        …
    }

    public Font getFont(String role) {
        …
    }
}
\$\endgroup\$
4
\$\begingroup\$

The flag was raised here:

public class ResourceManager {

A FoobarManager class is like a FoobarHelper: it quickly grows hair and tentacles, and becomes a dumping bag for pretty much anything that's remotely related to "resources" in general, and then it presents an interface that's constantly changing: now you have a getImage method, a ResourceBundle public field that's more powerful than what its name makes its users believe (Texts sounds quite limited, but then the ResourceBundle could contain literally any localized resource, so "Texts" could be a bit short, or misleading).

Then you have Preferences, which sounds like the ResourceManager is also responsible for exposing user settings.

This "Manager" is already well on its way to low cohesion and high coupling - two things you would normally want to strive to avoid in OOP code.

So, yup. It smells. =)

I don't like the public fields - even if they're final, they're public fields nonetheless. They should be exposed as property getters.

As for the "singleton" part, well, what you have is a type, with static members. That's not what a singleton is. A singleton is an object, that can only be instantiated once. An object has a type, but a type isn't an object: this is a "static class" (if that C# concept can be transposed to Java), not a "Singleton" - in fact, it's a "Noneton", because there's not even one single instance of your class around.. only its type.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.