2

I have a playlist class which contains a vector of song pointers.

class Playlist {
public:
    Playlist();
    Playlist(string);
    vector<Song*> GetSongList() const;
private:
    vector<Song*> songs;
    string name;
};

The function GetSongList() is as follows:

vector<Song*> Playlist::GetSongList() const {
    return songs;
}

In my main method I have a vector of playlist pointers: vector<Playlist*> playlists

I want to remove a song pointer from a specific playlist, but I'm having trouble. This is my current code to remove the indicated song:

Playlist* desiredPlaylist = playlists.at(index);
vector<Song*> temporarySongList = desiredPlaylist->GetSongList();
cout << "Pick a song index number to delete: ";
cin >> index;
temporarySongList.erase(tempSongList.begin() + index);

When I erase the song from the vector and re-output the contents of playlists.at(index).GetSongList() the song is not removed. I think the reason why is that calling GetSongList() does not retrieve the actual song vector, but just returns a copy, so I'm not altering the original vector. How do I directly remove the song from the original?

Ryan
  • 169
  • 6
  • 5
    Add a member function `Playlist::RemoveSong` that removes the song from the vector? – Some programmer dude Mar 08 '18 at 06:36
  • 1
    Unrelated: Examine why you are storing pointers to songs and make sure you are familiar with the [Iterator invalidation rules](https://stackoverflow.com/questions/6438086/iterator-invalidation-rules) for whatever container is storing the objects you're pointing at. – user4581301 Mar 08 '18 at 06:59

3 Answers3

2

Use a member function remove_playlist to remove the song from the playlist. std::list (in place of vector) is recommended since frequent delete, move and insert is required within the playlist. Also use smart-pointer to avoid memory leaks like this std::list<std::shared_ptr<Song>>

void Playlist::remove_playlist(int index)
{
  if( index < songs.size() )
   {
    auto v = songs.at(index);
    songs.erase(std::remove(songs.begin(), songs.end(), v), songs.end());
   }
}
seccpur
  • 4,996
  • 2
  • 13
  • 21
  • Pretty sure you can simplify that to `songs.erase(songs.begin()+index);` with a test to ensure `index` is within range. – user4581301 Mar 08 '18 at 07:01
  • how about if multiple playlists are referring to the same song to be removed? – Joseph D. Mar 08 '18 at 07:11
  • Finding the position in the list in O(N), so it isn't immediately clear that list is a better choice than vector. – juanchopanza Mar 08 '18 at 07:19
  • 1
    For multiple container instances holding pointers to the same item instance, `std::shared_ptr<>` is advisable. – user2328447 Mar 08 '18 at 07:19
  • 1
    @codekaizer: std::shared_ptr is the way to go if multiple songs in playlist. Above, just a pseudo code to realize OP issue first hand. – seccpur Mar 08 '18 at 07:32
  • @juanchopanza, this goes to the discussion which operation should be faster: search, insert or delete. – Joseph D. Mar 08 '18 at 07:42
1

You are right, the problem is caused by returning a copy.

You can either return a pointer

vector<Song*>* Playlist::GetSongList() {
    return &songs;
}

or a reference

vector<Song*>& Playlist::GetSongList() {
    return songs;
}

to your playlist.

A pointer is preferrable when it may happen, that there is no song list available and you thus sometimes have to return nullptr. So not in your example, because the member playlist is always available.

In most other cases, returning a reference is preferrable. Note that the method is not marked as const, because accesses to the returned vector<Song*>& alter the Playlist instance.

Another technique is to not return the member at all, but instead use a delegate to change the member.

void Playlist::ChangeSongList( const std::function<void(vector<Song*>&)>& fn ) {
    fn(songs);
}

This has the benefit that the class which supplies the member can lock the access to the member in a threaded environment and is better informed when the member is changed (e.g. for debugging purposes - ChangeSongList could dump the contents before and after calling fn())

Anyway, returning a copy has often also performance problems and thus is often not preferrable. If you want to return a const member, you should use a const reference instead:

const vector<Song*>& Playlist::GetSongList() const {
    return songs;
}

Please note that the answer seccpur gave is the most preferrable option in everyday life - a class should and usually takes care of its members itself, and don't let some outside code handle it. But this answer doesn't describe the difference in returning copies, pointers and references.

user2328447
  • 1,807
  • 1
  • 21
  • 27
  • so how do you remove a song from the play list? – Joseph D. Mar 08 '18 at 07:08
  • Well... like Ryan already did it: `erase(songList.begin() + index)`. But don't forget to bounds check before (or use `vector::at()`, which does that for you, in combination with `try/catch`). And don't ever forget to `delete` (or - better - use smart pointers instead) – user2328447 Mar 08 '18 at 07:14
  • adding the & gives me a new error: 'return': cannot convert from 'const std::vector>' to 'std::vector> &' – Ryan Mar 08 '18 at 07:24
  • Yes, I already wrote the reason in my answer: "Note that the method is not marked as `const`, because accesses to the returned `vector&` alter the `Playlist` instance.". You have to remove the `const` from the method declaration, because it is de facto not `const` anymore. – user2328447 Mar 08 '18 at 07:28
  • Oops... but I forgot to remove the `const` in the pointer return, fixed. – user2328447 Mar 08 '18 at 07:32
  • Might as well make the data member public. – juanchopanza Mar 08 '18 at 09:20
0

Quote

 Playlist* desiredPlaylist = playlists.at(index);
  vector<Song*> temporarySongList = desiredPlaylist->GetSongList();
  cout << "Pick a song index number to delete: ";
  cin >> index;
  temporarySongList.erase(tempSongList.begin() + index);

You are creating a copy by doing this 

vector<Song*> temporarySongList = desiredPlaylist->GetSongList();

So you are erasing from your local copy.

Consider changing

vector<Song*> GetSongList() const { return songs };

to 

vector<Song*>& GetSongList() { return songs } ;
jswl
  • 82
  • 5