Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have made a method which returns a list of all songs on my device. Now I want to get a list of albums out of these songs.

What I basically do is go through every song I have in my list, check if its album is already in the list and if not I create a new album.

This method will take some time since there are 1400 songs on my device so I wondered how I can make this method any faster? Also, any other improvements?

public static ArrayList<Album> loadAlbums( Context c ){
    ArrayList<Song> tmpList = getAllSongs( c);
    ArrayList<Album> albumList = new ArrayList<Album>();

    for( Song s : tmpList ){

        boolean added = false;

        for( Album a : albumList ){

            if( a.getAlbumName().contentEquals( s.getAlbum() ) && a.getArtistName().contentEquals( s.getArtist() ) ){
                a.addSong( s.getPath() );
                added = true;
            }
        }


        if( !added ){
            Album a = new Album( s.getAlbum() );
            a.setAlbum_id( s.getAlbumId() );
            a.setArtistName( s.getArtist() );
            a.addSong( s.getPath() );

            albumList.add(a);
        }

    }

    tmpList = null;
    return albumList;
}
share|improve this question
add comment

2 Answers 2

What you need is a Map. A map is a collection of key --> value combinations. There can not be two equal keys pointing to different values.

There are a few ways to organize this map. The simplest is to make Map<String, Album> albumNames;, but that would make it possible for collisions if two albums had the same album name.

Another alternative is Map<String, List<Album>> for fixing the problem with albums having the same name.

There are more alternatives but they can get quite complex so I think this is enough for now.

Then when you loop through your songs, for each song:

  • List<Album> possibleAlbums = albumNames.get(song.getAlbum()).
    • If that returns null, then add a new list and add an album to it.
    • otherwise loop through the list to find the album you are looking for (that matches the artist). Again, if none is found create a new one.
  • Once you have found or created a matching album, you can add the song to it.

Other comments:

  • Declare by interface and not implementation. Make the method return List<Album>, also use for example List<Song> tmpList = getAllSongs(c);

  • Use longer and better names. s --> song, a --> album, c --> context, tmpList --> songs.

  • tmpList = null; in the bottom of the method will have no effect whatsoever. You can safely remove that line.

share|improve this answer
add comment
  • The loadAlbums method should probably take a list of songs as its parameter instead of the less meaningful Context object.

  • The name getAlbum makes me thing that the method is going to return an object of type Album, so that should be renamed to getAlbumName().

  • You don't want to be constructing an album object by setting each of the object's properties line-by-line. Instead, add a static method on the album class for constructing an album object from a song:

    Album a = Album.GetAlbumFromSong(s)
    
  • Use a Map object to map between Albums and an ArrayList of songs on that album: HashMap<Album, ArrayList<Song>> albums. Then you can override equals and hashCode for type Album to test album equality using artist name and album name, then you can write:

    for(Song song: tmpList){
        Album inspectionAlbum = Album.GetAlbumFromSong(song);
        if(albums.containsKey(inspection)){
            albums.get(inspectionAlbum).add(song);
        }
    }
    

    The amount of time it takes to look up keys in your hashmap won't depend on size which should make this method a lot more performant for large input sets.

  • tmpList should be renamed to songs.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.