Searching for songs on an Android application

In my application I need to search for some songs on my external storage card every now and then. On startup I read everything, however when it comes to new activities I thought I could just pass the file paths and read the songs I need again.

I've created this function:

public static Song getSong(Context c, String songPath) {
    Cursor audioCursor = c.getContentResolver().query(
            MediaStore.Audio.Media.EXTERNAL_CONTENT_URI,
            new String[] { "*" }, MediaStore.Audio.Media.DATA + " = " + songPath, null, Media.TITLE + " ASC");

    if (audioCursor != null) {
        if (audioCursor.moveToFirst()) {

                String path = audioCursor.getString(audioCursor
                        .getColumnIndex(MediaStore.Audio.Media.DATA));
                String title = audioCursor.getString(audioCursor
                        .getColumnIndex(MediaStore.Audio.Media.TITLE));
                String album = audioCursor.getString(audioCursor
                        .getColumnIndex(MediaStore.Audio.Media.ALBUM));
                String artist = audioCursor.getString(audioCursor
                        .getColumnIndex(MediaStore.Audio.Media.ARTIST));
                int album_id = audioCursor.getInt(audioCursor.getColumnIndex(MediaStore.Audio.Media.ALBUM_ID));
                int trackid = audioCursor.getInt(audioCursor.getColumnIndex(MediaStore.Audio.Media.TRACK));
                Long duration = audioCursor.getLong(audioCursor.getColumnIndex(MediaStore.Audio.Media.DURATION));

                Song s = new Song();
                s.setPath(path);
                s.setTitle(title);
                s.setArtist(artist);
                s.setAlbum(album);
                s.setAlbumId(album_id);
                s.setTrackId(trackid);
                s.setDuration( new Duration(duration) );

                return s;
        }
    }
    return null;
}

This function would either return null if not found or the song object created from the file's data.

-

In general the code seems fine and it is not horrible or anything, but I think you could surely have improvements.

High level

Currently your code returns null if audioCursor == null, I think this should be an exception, possibly an existing one, or a custom one. I'm not exactly sure what it does, so I cannot give more advice.

General coding style

I suggest you to change your coding style to more consistent and I think you can enhance readability with some simple changes.

Take a look at:

Cursor audioCursor = c.getContentResolver().query(
        MediaStore.Audio.Media.EXTERNAL_CONTENT_URI,
        new String[] { "*" }, MediaStore.Audio.Media.DATA + " = " + songPath, null, Media.TITLE + " ASC");

I understand that CodeReview is quite limited in width and your IDE is not necessarily so limited, but since you decided to split up the arguments, I would prefer it like this:

Cursor audioCursor = c.getContentResolver().query(
    MediaStore.Audio.Media.EXTERNAL_CONTENT_URI,
    new String[] { "*" }, 
    MediaStore.Audio.Media.DATA + " = " + songPath, 
    null, 
    Media.TITLE + " ASC"
);

This way all arguments are clearly visible and it looks consistent.

Similar arguments hold for:

String path = audioCursor.getString(audioCursor
        .getColumnIndex(MediaStore.Audio.Media.DATA));

Here you are breaking up one argument, I would prefer to break it up outside, like this:

String path = audioCursor
        .getString(audioCursor.getColumnIndex(MediaStore.Audio.Media.DATA));

Then lastly your blanks are off on this line:

s.setDuration( new Duration(duration) );

I'd say there is no reason to add them, it can even create confusion as now I start to think about the reason of the blanks, this is more clear:

s.setDuration(new Duration(duration));

Early exiting

Currently you have all your code in the if (audioCursor != null) statement like this:

if (audioCursor != null) {
    //your code
    return value;
}
return null;

This leads to clutter and less space to actually type code because of the indentation, the preferred way would be to do so:

if (audioCursor == null) {
    return null;
}
//your code
return value;

This way you exit early and the rest of your code is not impacted by this flow.

Variable naming

Most variable names look okay, except Context c and Song s, I see no reason to not use the full name like context and song, it will not harm anybody.

When taking a closer look, I actually noticed that album_id and trackid are also off, they are not consistent and neither in the recommended camelCase, they should be albumId and trackId.

Code safety

I would recommend to make Context context and String songPath final, I would say it is important for context, such that you cannot accidentally reassign the context variable.

Performance note

Currently you create a new String[] { } on every method call, for maximum performance you could create a private final static String[] STAR_STRING_ARRAY = new String[] { } and reference that one such that you do not need to create a new string array at every call. It depends on the amount of calls this method receives to see whether it is really worth it.

One more high level look

After analyzing the code I see it can be decomposed into two methods:

• public static Song getSong(Context context, String songPath) { ... }

• private static Song getSong(Cursor audioCursor) { ... }

Where the latter would be called once you successfully have obtained the audioCursor and that one would do the low level job.

Conclusion

My conclusion still is that the code overall is fine minus the coding style issues, but these improvements would improve it.

I think it is also better than a Singleton class, because this to me simply looks like an utility method, hence warranting a place in a AudioUtils class, but I do not see a reason for a Singleton class for this.

StackExchange Code Review Q#56228, answer score: 4