Sound manager for Android

Question

I created a singleton class for managing sound effects on Android. This class will only be instanced and loaded once at the beginning, and each activity will use the loaded songs.

I don't know either if this is the good approach for Singleton, nor if this is the good way to play sounds in Android. This is working like a charm, and I'm wondering about the resource utilizations.

public class SoundManager {
 private static SoundManager mInstance;
 private SoundPool mSoundPool;
 private HashMap<Integer, Integer> mSoundPoolMap;
 private Vector<Integer> mAvailibleSounds = new Vector<Integer>();
 private Vector<Integer> mKillSoundQueue = new Vector<Integer>();
 private Handler mHandler = new Handler();
 private boolean mMuted = false;
 private static final int MAX_STREAMS = 2;
 private static final int KILL_AFTER = 3000;
 public static final int SOUND_SELECT = 0;
 public static final int SOUND_LOCKED = 1;
 @SuppressLint("UseSparseArrays")
 private SoundManager(Context context) {
 mSoundPool = new SoundPool(MAX_STREAMS, AudioManager.STREAM_MUSIC, 0);
 mSoundPoolMap = new HashMap<Integer, Integer>();
 loadSounds(context);
 }
 public static SoundManager getInstance(Context context){
 if(mInstance == null){
 mInstance = new SoundManager(context);
 Log.d("SPARTA", "Instanciation");
 }
 return mInstance;
 }
 /**
 * Load all sounds and put them in their respective keys.
 * @param context
 */
 private void loadSounds(Context context){
 addSound(context, SOUND_SELECT, R.raw.metallic_knock);
 addSound(context, SOUND_LOCKED, R.raw.licorice);
 }
 /**
 * Put the sounds to their correspondig keys in sound pool.
 * @param context
 * @param key
 * @param soundID
 */
 public void addSound(Context context, int key, int soundID) {
 mAvailibleSounds.add(key);
 mSoundPoolMap.put(key, mSoundPool.load(context, soundID, 1));
 }
 /**
 * Find sound with the key and play it
 * @param key
 */
 public void playSound(int key) {
 if(mMuted)
 return;
 //If we have the sound
 if(mAvailibleSounds.contains(key)) {
 //We play it
 int soundId = mSoundPool.play(mSoundPoolMap.get(key), 1, 1, 1, 0, 1f);
 mKillSoundQueue.add(soundId);
 //And schedule the current sound to stop after set milliseconds
 mHandler.postDelayed(new Runnable() {
 public void run() {
 if (!mKillSoundQueue.isEmpty()) {
 mSoundPool.stop(mKillSoundQueue.firstElement());
 }
 }
 }, KILL_AFTER);
 }
 }
 /**
 * Initialize the control stream with the activity to music
 * @param activity
 */
 public static void initStreamTypeMedia(Activity activity){
 activity.setVolumeControlStream(AudioManager.STREAM_MUSIC);
 }
 /**
 * Is sound muted
 * @param muted
 */
 public void setMuted(boolean muted) {
 this.mMuted = muted;
 }
}

• FR073N: Here is the corrected version : gist.github.com/FR073N/9902559

Answer 1

I totally agree with Ocelot20, but wanted to point out some more things and provide some comments of my own about some things that has been already mentioned:

About mAvailableSounds:

• Vector is deprecated, use List (interface) and ArrayList (implementation) instead.

  private List<Integer> mAvailibleSounds = new ArrayList<Integer>();
  

• Does the order of the sounds matter in the mAvailableSounds list? No. Use a HashSet instead:

  private Set<Integer> mAvailibleSounds = new HashSet<Integer>();
  

• Discard the mAvailibleSounds variable entirely and use mSoundPoolMap.containsKey() instead.

Mapping int to int...

You have used @SuppressLint("UseSparseArrays") to suppress a warning. However, you should know that there is some performance benefit in using a SparseArray rather than a HashMap<Integer, Integer>.

You create a mapping from integer to integer, by using your own keys. I don't see the need of this, honestly. Instead you can declare the R.raw.metallic_knock and similar as constants, such as:

public static final int SOUND_KNOCK = R.raw.metallic_knock;

Flexibility

Your class seems to be coded in a way intended to be quite flexible, which would be good, but this code is limited to your project only:

private void loadSounds(Context context){
 addSound(context, SOUND_SELECT, R.raw.metallic_knock);
 addSound(context, SOUND_LOCKED, R.raw.licorice);
}

If you would like to make another project and use this class, you would have to copy it and change those values. That's not a good idea. Programmers hate copy-pasting code within their projects. This class could be made more flexible by not making it dependent of which project is using it. The best way to do that would be to store the constants in a project-specific class, and that the project calls your SoundManager class with the resource-ids (such as R.raw.licorice).

Currently, if you've forgotten to call addSound for a sound but try to play it directly with playSound, you wouldn't hear anything. You could save yourself some debugging time from making it either log a warning or try to play a sound with that id. Using the R.raw values directly would help with this.

Some final words

As you've said yourself: This is working like a charm. Your code works, be happy! Really, I mean it.

• FR073N: Indeed, mAvailableSounds was useless. I get rid of that and used mSoundPoolMap as a SparseIntArray instead. Flexbility wise, I created another class loading all my sounds.
• Simon Forsberg: @FR073N Good work! Welcome back whenever you need more code reviewed.

Answer 2

Let me start by saying I have very limited Android experience. That said, I did look into the SoundPool class a bit and have some questions/comments about your implementation:

1. If you aren't already aware of some of the reasons why people consider Singletons an anti-pattern, I suggest you do some research on the topic and make sure you're ok with the downsides.
2. Your class, for the most part, seems to be wrapping a SoundPool with a key value pair structure despite SoundPool already being a key value pair structure. I would imagine you could do away with most of this class and just use the SoundPool class directly.
3. If you don't want to use SoundPool directly for some reason, why not at least use the soundId as a key? It already appears to be unique, and it's what the calling code probably wants anyway. If not, you may want to do some checking to make sure that duplicate keys aren't entered, or take the key assignment out of the hands of the caller and simply return an id as the result of the addSound function.
4. Using both mSoundPoolMap and mAvailibleSounds means you have to manage keys in two locations which could lead to them getting out of sync or some confusion as to why there are two fields holding the same information. Using only mSoundPoolMap would be sufficient and avoid duplication. This highly voted question isn't quite the same, but it touches on a lot of good points why this kind of duplication can be bad.

Hope this helps!

• FR073N: Your answer pointed out what others have explained. I could have accepter your answer as well, but the others gave me a little more information. Just one thing, for point 3, I used mSoundPoolMap to prevent from using the SoundPool load method each time. But you're true when you say that mAvailableSounds had the same function.
• Ocelot20: I wasn't suggesting doing away with caching if it was necessary, I was suggesting that you don't need to create a new key when soundId already existed and provides a direct map to what the consumer probably already wants to find a sound by (since they ask for it by soundId also).

Answer 3

1. I agree with Simon that Vector is deprecated and should be changed but its thread-safety is used in the case of mKillSoundQueue (it's used by multiple threads), so be careful. If you change it make sure that it will remain thread-safe. The name of the field also suggests that it's a queue, so I'd probably go with a thread-safe Queue implementation here.

2. Just a question: wouldn't it be safer using soundId here too? (It have to be final for that.)

   if (!mKillSoundQueue.isEmpty()) {
    mSoundPool.stop(mKillSoundQueue.firstElement());
   }
   

   If I'm right the following is the same and you could get rid of the mKillSoundQueue field completely:

   mSoundPool.stop(soundId);
   

   SoundPool.stop says the following:

   If the stream is not playing, it will have no effect.

3. Using m prefixes to access fields not really necessary and it's just noise. Modern IDEs use highlighting to separate local variables from fields.

   private static SoundManager mInstance;
   

4. I'd use a _MILLIS (or anything which is appropriate) postfix here for better clarity:

   private static final int KILL_AFTER = 3000;
   

5. This:

   mSoundPoolMap = new HashMap<Integer, Integer>();
   

   could be in the same line as the declaration:

   private HashMap<Integer, Integer> mSoundPoolMap = 
    new HashMap<Integer, Integer>();;
   

6. It's really bad formatting:

   if(mMuted)
   return;
   

   Indentation of the return statement would make it readable.

7. According to the Code Conventions for the Java Programming Language

   if statements always use braces {}.

   Omitting them is error-prone.

8. Comments like this are just noise, so they're unnecessary, remove them:

   * @param key
   

   (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

9. Instead of comments like

   //If we have the sound
   if(mAvailibleSounds.contains(key)) {
   

   and

    //And schedule the current sound to stop after set milliseconds
   mHandler.postDelayed(new Runnable() {
   

   You could create explanatory variables and methods.

   boolean hasSound = mAvailibleSounds.contains(key);
   if (hasSound) {
    ...
   

   and scheduleCurrentSoundStop().

   References:

   • Clean Code by Robert C. Martin, Bad Comments, Don’t Use a Comment When You Can Use a Function or a Variable, p67
   • Clean Code by Robert C. Martin, G19: Use Explanatory Variables
   • Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable

10. In the first case above I'd use a guard clause to make the code flatten:

    boolean hasSound = mAvailibleSounds.contains(key);
    if (hasSound) {
     return;
    }
    ...
    

• FR073N: Your answer and Simon's really improved my code. I accepted his answer because I learned something about singleton with Android, but I could have accepted yours as well. Just one last question, about the third point : when should we use the m prefix ?
• palacsint: @FR073N: Sure, thanks for the feedback! (If you have time feel free to help graduating the site. I'm sure that you can find another useful questions and answers on the site to vote on).
• palacsint: @FR073N: I don't know any valid use-cases for the m prefix right now, nothing comes to mind.

Answer 4

Unlike Ocelot20, I have quite a lot of Android experience, and this answer will focus on only one thing, namely:

Getting rid of the Singleton pattern in Android

Quote from a Stack Overflow answer:

Although the app context can be considered a singleton itself, it is framework-managed and has a well defined life-cycle, scope, and access path. Hence I believe that if you do need to manage app-global state, it should go here, nowhere else. For anything else, rethink if you really need a singleton object, or if it would also be possible to rewrite your singleton class to instead instantiate small, short-lived objects that perform the task at hand.

In your Android.XML file there is the tag <application>. This tag can have the property android:name. This property can be set to a classname that extends android.app.Application.

<application android:name="com.mypackage.MyApplication" ...>

The com.mypackage.MyApplication class is very easy to make, simply this would be enough:

public class Register extends Application {
}

But that's kind of boring, so you'd probably would want a public void doSomething() method, or - in your example:

public class Register extends Application {
 private SoundManager soundManager;
 public SoundManager getSoundManager() {
 if (soundManager == null) {
 // Application extends Context so just pass along 'this'!
 soundManager = new SoundManager(this); 
 }
 return soundManager;
 }
}

How to get an instance of the MyApplication class then?

When your application starts, if you have specified the android:name property then Android will create an instance of the classname that you put here. This instance will be available through the Android Context that gets passed around to all Views, Activities and Fragments.

That said, it is very easy to get access to the instance of com.mypackage.MyApplication from more or less anywhere, whether you have an Activity, a View or a Fragment. (If you haven't used Fragments yet by the way, you really should - there is a compatibility library available if you are targeting API <= 10).

All your Android Activities have the getApplication method, this retreives an instance of an Application class, in your case it will return the MyApplication instance.

All Android Views have the getContext method, which has a getApplicationContext method which will return an Application instance, again; this will be your MyApplication instance.

Examples

In a method in your activity:

MyApplication myApp = (MyApplication) getApplication();
myApp.doSomething();

When you have no Activity nearby but instead have a View or another way to get a Context:

MyApplication myApp = (MyApplication) getContext().getApplicationContext();
myApp.doSomething();

In a fragment, use either getView or getActivity and then use one of the methods described above (getApplication or getContext().getApplicationContext())

That, my friend, is how you avoid making your own singletons in Android. This is the recommended way of doing it, and it can also be used for passing data from one activity to another (when you are unable to use getExtra and putExtra to pass data between activities).

• FR073N: I wasn't aware of that way. This answers my problematique of creating a singleton class in Android.