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);
}
}
2 Answers 2
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) {
...
}
}
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.