Improving speed of Animation class

Question

This is a class to hold an animation and be able to refer to different animations by titles, but it seems like calling draw(Graphics2D g) and update every frame, would, with this implementation, cause a big slowdown with all the list.get(Object obj) calls. Is this a big deal, and if so how should I change the implementation? Other comments on the code are very welcome.

package core.graphics;
import java.awt.Graphics2D;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
public class AnimationSet {
 private int currentAnimation;
 private Map<String, Integer> namesToAnimation;
 private List<Animation> animations;
 public AnimationSet() {
 currentAnimation = 0;
 namesToAnimation = new HashMap<String, Integer>();
 animations = new ArrayList<Animation>();
 }
 public void addAnimation(String title, Animation animation){
 animations.add(animation);
 namesToAnimation.put(title, animations.size()-1);
 }
 public void goToAnimation(String title){
 currentAnimation = namesToAnimation.get(title);
 }
 public void removeAnimation(String title){
 animations.remove(namesToAnimation.get(title));
 namesToAnimation.remove(title);
 }
 public void draw(Graphics2D g, int x, int y){
 g.drawImage(animations.get(currentAnimation).getImage(), x, y, null);
 }
 public void update(){
 animations.get(currentAnimation).update();;
 } 
}

Answer 1

As your List is instantiated as an ArrayList, the lookup time for get is constant time, \$O(1)\$.

There is however, somethings that I can point out.

Final

Whenever possible, add the keyword final to member fields of your class. This keyword should be used for values/references that never change. Your namesToAnimation and animations variables can be marked with final.

Why Map<String, Integer>?

You seem to only use your namesToAnimation as a lookup for String to index in the 'animations' list. You never increase/decrease your currentAnimation value. Then why do you use Map<String, Integer>? I think you can get rid of your list entirely and use Map<String, Animation>

Rewritten code according to what I have mentioned above:

public class AnimationSet {
 private Animation currentAnimation;
 private final Map<String, Animation> animations;
 public AnimationSet() {
 animations = new HashMap<>();
 }
 public void addAnimation(String title, Animation animation){
 animations.put(title, animation);
 }
 public void goToAnimation(String title){
 currentAnimation = animations.get(title);
 }
 public void removeAnimation(String title){
 animations.remove(title);
 }
 public void draw(Graphics2D g, int x, int y){
 g.drawImage(currentAnimation.getImage(), x, y, null);
 }
 public void update(){
 currentAnimation.update();
 } 
}

Usefulness

I'm sorry, but I can't really understand why you have this class? What functionality does it provide to you? Would it be easier to have the same functionality without this class?

Hopefully you have a good use case for this class, in which case, great! I'm just not sure how I myself would use this class, which brings me to the next point:

Documentation

Although your naming is really good (you have no idea how happy I am to be able to say that!), your class could use some documentation in the form of JavaDoc. Stating the purpose of the class and the usage of the class and it's methods in JavaDoc helps any potential users of your class, it can even help yourself several times.

Thread safety

Your code would likely cause problems if it would be used from multiple threads. To avoid those problems, learn about Java Concurrency. If you're not using multiple threads now, you might be later, that's why I'm mentioning this. It's something for you to learn in the future :)

• Ingo Bürk: Good answer, as an additional point: I'd initialize final members right where they are declared if they don't depend on constructor parameters.