CODEWITHSUNDEEP
c++insertion-sort
Potion
Potion

Reputation: 946

Insertion Sort: What am I doing Wrong?

I am trying to do an insertion sort. The song is a simple structure containing an Artist and Title property. I call the CompareTitle(Song& s1, Song& s2) which returns true if the song title of the first song is before the second song's title. The sort condition seems to work fine when the swapping portion of the code is commented out. When not, I get this error. Im not sure how to approach it:

playlist.cc:192:22: error: object of type 'Song' cannot be assigned because its copy assignment operator is implicitly deleted *itr = j;

//do insertion sort
for(auto itr = newSongList.begin(); itr != newSongList.end(); ++itr)
{

    for(auto jtr=itr; jtr != newSongList.begin(); --jtr){
        cout << itr->GetTitle() << " " << jtr->GetTitle() << endl;
        // if s1 is not before s2 then swap them
        if(!Song::CompareTitle(*itr, *jtr)){
            cout << "Swap True (" << itr->GetTitle() << "," << jtr->GetTitle() << " )"<< endl;
            Song i = Song(itr->GetTitle(),itr->GetArtist());
            Song j = Song(jtr->GetTitle(), jtr->GetArtist());

            *itr = j;
            *jtr = i;

            cout << "Swap After (" << jtr->GetTitle() << "," << itr->GetTitle() << endl;
        }
    }
}

Below is the structure of the song and playlist:

#ifndef PLAYLIST_H
#define PLAYLIST_H

#include <functional>   // For std::function
#include <string>
#include <list>

using namespace std;

extern void SongCallback();

class Song {
  public:
    explicit Song(const string& title, const string& artist,
        const function<void()> = &SongCallback);
        const string& GetTitle() const;
        const string& GetArtist() const;
        bool operator==(const Song& s) const;
        bool operator()(const Song& s) const;

        static bool CompareTitle(const Song& s1, const Song& s2);
        static bool CompareArtistTitle(const Song& s1, const Song& s2);

private:
    const string title_;
    const string artist_;
    const function<void()> callback_;
};

class Playlist {
public:
    explicit Playlist() {}
    void AddSong(const string& title, const string& artist);
    unsigned int RemoveSongs(const string& title, const string& artist);
    list<Song> PlaylistSortedByTitle() const;
    list<Song> PlaylistSortedByArtistTitle() const;
    unsigned int NumSongs() const;
    unsigned int NumSongs(const string& artist) const;

private:
    list<Song> songs_;
};

#endif // PLAYLIST_H

Upvotes: 0

Views: 108

Answers (2)

Saeed
Saeed

Reputation: 152

Since, you have const title_ and artist_ params, you can't assign and overwrite these using assignment operator.

The compiler will also not generate assignment operator. Since the purpose of an assignment operator is to alter members after construction, it doesn't make sense to generate an implicit assignment operator when one of the member can never be altered. The compiler refuses to try and guess what you want it to do and forces you to provide your own assignment operator with the semantics you want.

I think instead, what you can do is manipulate the list entries using list::insert and list::erase methods, like I've shown below to achieve your desired result.

for(auto itr = newSongList.begin(); itr != newSongList.end(); ++itr)
{

    for(auto jtr=itr; jtr != newSongList.begin(); --jtr){
        cout << itr->GetTitle() << " " << jtr->GetTitle() << endl;
        // if s1 is not before s2 then swap them
        if(!Song::CompareTitle(*itr, *jtr)){
            cout << "Swap True (" << itr->GetTitle() << "," << jtr->GetTitle() << " )"<< endl;
            Song i = Song(itr->GetTitle(),itr->GetArtist());
            Song j = Song(jtr->GetTitle(), jtr->GetArtist());

            newSongList.insert(itr, j); // extend list by inserting j song before itr song
            newSongList.erase(itr);     // itr is still pointing to original song
            newSongList.insert(jtr, i); // extend list by inserting i song before jtr song
            newSongList.erase(jtr);     // jtr is still pointing to original song

            cout << "Swap After (" << jtr->GetTitle() << "," << itr->GetTitle() << endl;
        }
    }
}

Upvotes: 0

Quinn Mortimer
Quinn Mortimer

Reputation: 721

It would seem that every time you should be swapping iterator the two elements, you are instead adding elements to the container before both itr and jtr.

for(auto itr = newSongList.begin(); itr != newSongList.end(); ++itr)
{

    for(auto jtr=itr; jtr != newSongList.begin(); --jtr){
        cout << itr->GetTitle() << " " << jtr->GetTitle() << endl;
        // if s1 is not before s2 then swap them
        if(!Song::CompareTitle(*itr, *jtr)){
            swap(*itr, *jtr);
        }
    }
}

Here you can either use std::swap as the swap function, or write your own function to switch the titles and artists of the songs, along with any other members that may be added to them. std::iter_swap could also be used as an alternative.

std::swap on CPP reference

std::iter_swap on CPP reference

Upvotes: 1

Related Questions