Storing images in a "static" class?

Newbie & Debugging Questions
#1

I am right now storing all my images in a arbitrary class in static fields since I will never manipulate the images and I really only need one instance of each image. So every wall tile would reference the same wall image. Example:


public class Storage {
  public static BufferedImage wallSprite;
  public static BufferedImage otherSprite;
}

public class Tile {
  protected BufferedImage sprite;
}

public class Wall extends Tile {
  public Wall() {
    sprite = Storage.wallSprite;
  }
}

Are there better ways of storing images? Is this even something that shouldn’t be done at all? It just seems so convenient.

#2

This Thread might be of interest to you http://www.java-gaming.org/topics/loading-multiple-images/24878/msg/212071/view.html

#3

Why not just put them right into the class that actually needs the images ?
Why is one sort of images kept in Storage while the other in Tile ?

#4

@MashedPotatoe
they are many thing to so this, but what you do is pretty useful and good.
When it gets crazy with many many images you will do arrays with indexes or hashmaps in this static storage class

#5

Yes, a HashMap is certainly better. I actually use one already. Just for explaining purposes I chose to use this format. :wink:

@65k
The image in the Tile class is actually only a reference to the static image in the storage. Whenever I would create a new tile of some sort it would point the sprite reference to the correct static image that has already been loaded.
And I have thought about putting them in the classes directly. But right now I am loading everything at startup and reference it when it is needed. If I was to put the images directly into the classes wouldn’t I need to load these fresh image (or get subImages from a spriteSheet) every time I created a new object of say Tile? So now every Tile has a real image instead of just a reference to it.

Or I could put it into the corresponding class and make it static there.

@jonjava
still reading through the thread

EDIT: After reading through the thread and watching the video I will instantiate the storage class once and pass it to every object needing it. Dependency injection is something I should get used to even though I am working alone and no one else is reading/using my code. Seems just like good programming practice and makes the code clearer. Thanks for all the input!

#6

Dependency injection is good to get rid of globals (which are public statics after all).
There is little usage of public static beyond simple constant data.

#7

On that thread I use Z-man second post way.

#8

That’s what I use now, too. Refactoring was a pain, but in the end my code is easier to read now.

#9

@65k,
So you are not a fan of singleton? I think static objects are perfectly fine as long as they are managed well.

Normally I use classes that I call managers to hold and manage global/static entities. For example I have a class called ImageManager that I use for loading and fetching all my images. To use ImageManager one would call a public static method called ImageManager.getInstance()… which returns the one existing instance. I have used this design also for dependency injection. For example the getInstance method might require some input, and depending on that input I return the appropriate object (which could be a completely different class that extends the same interface). As an example, I created EnemyUnitFactory which is retrieved by calling EnemyUnitManager.getFactory(). Depending on what round/world the user selected they will get the appropriate EnemyUnitFactory for that world/round.

When messing around with static just be careful about controlling when objects get instantiated (or in this case when images get loaded). You want to have fine control over that as you do not want a user waiting for images to load just because you have instantiated them in a static scope.

thanks
jose

#10

“Manager” to me is a code smell. When I see a “Manager” suffix on classes, that’s usually a giveaway that responsibilities haven’t been well factored enough, in that the job of “FooManager” is “do miscellaneous random stuff with Foo that doesn’t seem to fit anywhere else”.

Now in this case, your manager classes look a bit like modules in Guice. But you should be really wary of slapping any more functionality on those that isn’t related to resolving factories.

#11

Well/Hell, no, I’ve never been a fan of anything. The step up of fan is fan boy. ;D
Here, I was only referring to global variables, not to the singleton pattern. Why the latter is/can be problematic is wildly discussed on the internet.
I do use it though, for reading system properties. Why ? Because it was a cozy lazy shortcut and because it is used in many classes in all layers. Which shouldn’t be true for the average class like an ImageManager.
These static getInstance methods are the entrance to a dead end street. If I had to introduce for whatever reason another different property reader, the resulting refactoring would be cumbersome, if there was just a member variable set by dependency injection, the refactoring would not affect any of the property reader’s user classes. And the client classes had no idea of how to get a reader, what they don’t need and should know. They should just use it, nothing else. No dependency to object retrieval or creation.

#12

sproingie,

Thanks for the feedback. I think the managers fit a nice role in that it frees classes from having to meddle too much with certain types of objects… provides a simple interface. In my current game I created a manager for all the major classes (TurretManager, RoundManager, ProjectileManager, ImageManager, ScreenManager). Their roles are specific to the objects they are managing. In some cases acting as a pure factory or builder, facade, dependency injection, data model, or just being a static holder of the active object (TurretManager has a reference to the active Turret). What I like about this is that I do not have to decide if I need a facade here, or builder there, then have builders and factories, and context classes. But I like to get feedback and hear criticism.

Here are the public methods to the most involved manager, ProjectileManager… used primarily by, but not limited to, the Turret classes who has to use Projectiles extensively.

ProjectileManager:

//singleton
public static ProjectileManager getInstance()

//factory method to retrieve projectiles from a specific ProjectileType
public List<Projectile> getProjectiles(ProjectileType pt)

//The turret does not decide which ProjectileTypes are chosen, so he gets them from this manager
public ProjectileType[] getActiveProjectileTypes() {

//When a round starts the projectiles need to be notified and also need to be upgraded to last saved state under certain circumstance
public void startRound(ProjectileType[] projectileTypes, boolean retrieveProjectileState) {

//At the end of a round, if the user wins, the projectiles need to have their upgrades saved to the user's profile
public void saveUpgradesToProfile(ProjectileType[] projectileTypes) {

//A projectile registers all their upgradable abilities to this manager because they need to be fetched later on
public UpgradableAbility registerUpgradableAbility(Class pClass, UpgradableAbility ability) {

//fetches all registered upgradable abilities, used mainly by the upgrade screen where a user can upgrade their projectiles
public List<UpgradableAbility> getUpgradableAbilities(Class pClass) {

In this case I would say ProjectileManager is a facade (startRound), factory (getProjectiles), data model (registerUpgradableAbilities). Dividing this work up into various other classes was causing the code smell. For example, The UpgradeScreen needs to know about all the registered upgradable abilities. Passing this information from the Projectiles all the way to one of the Screens would be messy without some static source where to retrieve them from.

thanks
jose

#13

65k,

I hear what you are saying. One of the things that I hate doing is passing data model objects around. This really irks me. For example if I have some ObjectA that creates DataModelA and has a reference to ObjectB and ObjectB has a reference to ObjectC, and while ObjectB does not have a need for DataModelA ObjectC does. I hate having to pass references of DataModelA through ObjectB just to get it in the hands of ObjectC. Having a nice and clean static way to obtain DataModelA lessons the amount of needless references objects need to hold or pass around. Here are some more points…

  1. The fact that ObjectC needs DataModelA cannot be avoided and hence whether you pass it in the constructor or retrieve it from static method does not make ObjectC any more reusable in another project. You still cannot use ObjectC without DataModelA regardless of how you get ObjectC that reference.

  2. In some applications there are data models that are needed by many objects, hence going with static reduces the amount of code.

  3. I have yet ran into the issues that people say can happen if they use static, and I’m sure those who say it is bad have not ran into them either. The biggest of which was that someone else can alter the object… but this is easily managed.

  4. I still think that students should be told to avoid static because they might get some bad habits. Should only used by those who have avoided them for a long period of time, have learned the principles of encapsulation and lose-coupling/tight-cohesion, and over time has realized that they can use static safely.

#14

This image loading and storing problem should go to wiki :slight_smile:

#15

With 4) I totally agree.

But I don’t get why you see dividing the ProjectileManager as a code smell.
If I understand it correctly I think it has too many responsibilities, namely the upgrade stuff mixed with lower level projectile firing.

I fear naming a class *Manager tempts you into putting too much into it: “Oh, it’s a mighty manager, he can do it as well”
Just think of the real world and what managers really do ;D

Passing stuff around can be cumbersome, but it helps in encapsulating and isolating things and eventually it is good for maintainability (of which I am indeed a big big fan ;D)

#16

Ha! :smiley:

#17

65k,

Thanks for the feedback. I will not change the current arch but will be more critical of any pitfalls that it can lead to based on your feedback. If ProjectileManager sees that another class is needed to manage upgrades, then one can be created to encapsulate that further (there already is a class or two that handle that), and none of the other classes that deal with ProjectileManager need to change… this is the beauty of encapsulating all Projectile related interfaces to as few classes as possible (the Projectile itself and the manager). Projectile and its manager can then decide how to break up its responsibilities across any other classes that might be needed.

thanks
jose