Operator overloading in C++ to manage playlists of music

Question

I've built this little C++ program just to take some rust off my coding skills since it's been ages since the last time I coded something.

The purpose is to organize my music, so I've built the classes 'song' and 'playlist' with some methods which could come in handy - maybe - such as printing on file the list of songs in the playlist and so on. I'm not sure that std::vector is the right choice to store the songs in the playlist (as private member). Would a std::set be more efficient?

Given a list of songs I wanted to sort out if each and every one of the songs in the list appeared at least in one playlist, with each playlist representing a musical genre.

I was also interested in listing the "overlaps" between two different playlists, to get a sort of Venn Diagram of my music. I've accomplished that by overloading the operators + and - for the playlist, and by defining a function "intersection".

I'm defenetly not sure about the best way to handle the const functions.

I've used the following libraries:

#include <fstream>
#include <map>
#include <vector>
#include <set>
#include <iostream>
#include <string>
#include <sstream>

Here it is the implementation of the class song:

class song {
private:
 std::string title;
 std::string author;
 std::string album;
public:
 song(std::string tit) : title(tit), author("? "), album("? ") { }
 song(std::string tit, std::string au, std::string al) : title(tit), author(au), album(al) { }
 const std::string gettitle() {return title;}
 const std::string getauthor() {return author;}
 const std::string getalbum() {return album;}
 const bool operator==(song & a) {
 if(title == a.gettitle()) {
 if(author == a.getauthor()) {
 if(album == a.getalbum()) {
 return true;
 }
 }
 }else{ return false; }
 }
 void playsong() { 
 std::cout << std::endl;
 std::cout << "\"" << title << "\"" << ", by " << author << ", \n";
 std::cout << "from the album \"" << album << "\"." << std::endl;
 }
 void showtitle() { std::cout << title << std::endl; }
 void showauthor() { std::cout << author << std::endl; }
 void showalbum() { std::cout << album << std::endl; }
 void showall() { 
 std::cout << title << " | " << author;
 std::cout << " | " << album << std::endl;
 }
};

And of the class playlist:

class playlist {
private:
 std::string name;
 std::vector<song> list;
public:
 playlist() : name(" ") {}
 playlist(std::string nam) : name(nam) {}
 playlist(playlist & x, std::string nam) {
 name = nam;
 list = x.getlist();
 }
 playlist operator=(const playlist & x) {
 list = x.getlist();
 name = x.getname();
 return *this;
 }
 const playlist operator+( const playlist & x) {
 playlist z(*this);
 for(int i=0; i<(x.getlist()).size(); i++) { 
 if (false == this->findsong((x.getlist())[i])){ 
 z.addsong((x.getlist())[i]);
 }
 }
 z.setname(this->getname() + "+" + x.getname());
 return z;
 }
 const playlist operator-(const playlist & x) {
 playlist z(*this);
 for(int i=0; i<(x.getlist()).size(); i++) {
 if (this->findsong((x.getlist())[i])){ 
 z.deletesong((x.getlist())[i]);
 }
 }
 z.setname(this->getname() + "-" + x.getname());
 return z;
 }
 std::vector<song> getlist() const { return list; }
 std::string getname() const { return name; }
 song getsong(std::vector<song>::iterator it) const { return *it; } 
 std::vector<song>::iterator getsong(song & x) {
 std::vector<song>::iterator it;
 it = list.begin();
 while (it != list.end()) {
 if(*(it) == x) break;
 it ++;
 }
 return it;
 }
 void setname(std::string nam) { name = nam; }
 void addlist(std::string file) {
 std::ifstream in(file);
 std::string line;
 while(std::getline(in, line)) {
 std::stringstream linestream(line);
 std::string tit, au, al;
 getline(linestream, tit, '\t');
 getline(linestream, au, '\t');
 getline(linestream, al);
 song x(tit, au, al); 
 this->addsong(x); 
 }
 in.close();
 }
 void addsong(song x) { list.push_back(x); }
 void deletesong(song x) {
 int cont=0;
 std::vector<song>::iterator it;
 it = list.begin();
 for(auto el : list) {
 if(el == x){
 list.erase(it);
 std::vector<song>::iterator it;
 it = list.begin() + cont;
 }
 cont ++;
 }
 }
 bool findsong(song x) {
 bool tobe = false;
 for (auto el : list) {
 if(el == x) tobe = true;
 }
 return tobe;
 }
 void showplaylist() {
 int cont=0;
 std::vector<song>::iterator it;
 it = list.begin();
 std::cout << std::endl << name << std::endl << std::endl;
 while (it != list.end()) {
 cont++;
 std::cout << cont << ") ";
 it->showall();
 it++;
 }
 }
 void playplaylist() {
 int cont=0;
 std::vector<song>::iterator it;
 it = list.begin();
 std::cout << std::endl << name << std::endl << std::endl;
 while (it != list.end()) {
 cont++;
 std::cout << cont << ") ";
 it->playsong();
 it++;
 }
 }
 std::ofstream printplaylist(std::string nam) {
 std::ofstream out(nam + ".dat");
 std::vector<song>::iterator it;
 it = list.begin();
 out << std::endl << name << std::endl << std::endl;
 while (it != list.end()) {
 out << it->gettitle() << "\t";
 out << it->getauthor() << "\t";
 out << it->getalbum() << std::endl;
 it++;
 }
 out.close();
 return out;
 }
};

This is the only function I wrote:

playlist intersection(playlist & x, playlist & y) {
 playlist z, q, t;
 z = x + y;
 t = x - y;
 q = y - x; 
 z = z - q;
 z = z - t;
 z.setname("(" + x.getname() + ")^(" + y.getname() + ")" );
 return z;
}

In the main I tried my methods to check if they worked fine. The playlist are "loaded" in my program through some files.dat wich have the following structure:

Title \tab Author \tab Album

Answer 1

Constructors

The better way to write the constructor is

song(std::string tit, std::string au = "? ", std::string al = "? ")
 : title(tit), author(au), album(al) { }

That way specifying the title and the author but leaving the album default is possible. Also you only need a single constructor now. You can further improve by using title(std::move(tit)) to skip a memory allocation.

Privacy

Through keeping title, author and album private and only providing functions like gettitle and showtitle you sort of made those read-only. Why? What is the danger having a song and setting the title of it? If someone wants to do it they can do

s = song{"new title", s.getauthor(), s.getalbum()};

which is just awkward. You didn't actually gain privacy while inconveniencing your users. I'd make all the members public and remove the getters until there is internal logic that keeps the members synchronized and you cannot guarantee it when users can change the members.

Warnings

Enable them. They are really useful. There is a bug in const bool operator==(song & a) but it's not really worth learning about because a compiler can spot it so you don't have to.

Const correctness

When comparing 2 songs those should not change and only being able to compare modifiable songs is weird. The correct signature for that is bool operator==(const song & a) const. The const on the right refers to *this, meaning when you do s1 == s2 then s1 is allowed to be const. The const in the middle refers to the right side, s2 in the example which we also want to allow to be const. The const in the front you had refers to the bool and besides very weird edge cases you don't want to return const objects by value. The same applies to gettitle and arguably those functions should return a const std::string & to avoid making a copy.

Comparisons

If you want lexicographic compares you can use

return std::tie(title, author, album) == std::tie(a.gettitle(), a.getauthor(), a.getalbum());

It doesn't look that much better than what you wrote, but it fixed the bug I hinted at when talking about warnings and when doing operator < this becomes much easier than doing it by hand. C++20 is not that far away and then defaulting operator <=> should make this convenient.

Separation of Concerns

song prints to std::cout. It really shouldn't. As a user of your class I should be the one who decides how and where things are printed to. I want to print to std::clog for example. For that you can add a function std::ostream &operator <<(std::ostream &os, const song &s) which prints its elements to os. Through inheritance I can now do std::cout << song{}; or std::fstream("/tmp/songs.txt") << song{}; or whatever I want. Although if you make the members public this is probably no longer necessary. Also users can add their own way of printing.

Extra Parenthesis

x.getlist() has lots of extra parenthesis around making the code less readable. I find findsong((x.getlist())[i]) particularly jarring. findsong(x.getlist()[i]) is better.

Yoda Conditions

if (false == this->findsong((x.getlist())[i])) should better be written as if (not findsong(x.getlist()[i])). Using not over ! is a style choice some people find weird and you don't need to follow it, but yoda conditions are harmful to readability and with warnings enabled have no upside.

• nwp: I don't have time for playlist right now, but maybe it's a start.
• agneau: Thank you for the suggestions, I'll start by improving the song class. If you have time later to check also the playlist class it would be awesome
• nwp: @Gitana The other answer pretty much covered that already, but I added 2 points I didn't see there. for (auto &element : container) is a generally useful idiom to use for iterating over elements of a container.

Answer 2

Single-responsibility: the show*() methods in song don't provide any benefit, since we already have suitable get*() methods which can be used more flexibly. Thus, song needn't require <iostream> or the like.

Returning const std::string here probably isn't what you meant:

const std::string gettitle() {return title;}
const std::string getauthor() {return author;}
const std::string getalbum() {return album;}

More useful would be

std::string gettitle() const {return title;}
std::string getauthor() const {return author;}
std::string getalbum() const {return album;}

This misplaced const also affects operator ==, which also has scope to be simpler and clearer. Any time we have an if/else where the branches return true or false consider just using a boolean expression directly for return:

bool operator==(const song& a) const
{
 return title == a.title
 && author == a.author
 && album == a.album;
}

We can also access the private members of a, since it's of our class.

Consider providing operator !=, too:

bool operator!=(const song& a) const
{ return !(*this == a); }

The constructors of song accept std::string by value; we should use std::move to avoid copying again (and we can use default arguments to reduce duplication):

explicit song(std::string title, std::string author = "? ", std::string album = "? ")
 : title(std::move(title)),
 author(std::move(author)),
 album(std::move(album))
{ }

(Note explicit, because we don't want this class to be considered for implicit conversion from string).

---

One of the playlist constructors doesn't initialise members, but uses assignment in the body instead. Prefer initialisers, and consolidate using default arguments:

explicit playlist(std::string name = "")
 : name(std::move(name)),
 list()
{}
playlist(const playlist & x, std::string name)
 : name(std::move(name)),
 list(x.list)
{}

Operator = should return a reference rather than a copy (g++ -Weffc++ catches this):

playlist& operator=(const playlist & x) {
// ^^^

But there's absolutely no need to declare this operator - follow the Rule of Zero, and let the compiler generate it and the copy/move constructors too.

Operator + performs set addition, which surprised me - normally, I would expect concatenation, like my existing Empeg Car player. Even if that's what's required, we can improve it. Any time we use a loop like for (std::size_t i=0; i < container.size(); ++i) and then only use i for indexing, we can replace with a range-based for loop:

playlist operator+(const playlist& x) const
{
 playlist z(*this, name + "+" + x.name);
 for (auto& song: x.list) {
 if (!findsong(song)) {
 z.addsong(song);
 }
 }
 return z;
}

This still scales poorly, as the linear search in findsong is performed for each song to be added. If we make song sortable or hashable, we could use a std::set to identify duplicates, at a cost of some overhead initialising the set.

I don't see why we have this method in the public interface:

song getsong(std::vector<song>::iterator it) const { return *it; }

That's something any calling code can do much more simply, and I'd argue that we probably shouldn't be giving out such iterators anyway.

The other getsong seems to exactly implement std::find(), and I don't see the value to users.

In removesong, we don't need to keep count with cont. The right way to cope with iterator invalidation is to use the return value from erase():

void deletesong(const song& x)
{
 auto it = list.begin();
 while (it != list.end()) {
 if (*it == x){
 it = list.erase(it);
 } else {
 ++it;
 }
 }
}

However, with that said, the real way to implement "remove all matching" is to use the Erase-remove idiom:

#include <algorithm>
void deletesong(const song& x)
{
 list.erase(std::remove(list.begin(), list.end(), x), list.end());
}

In a similar vein, findsong() can be simplified using std::find():

bool findsong(const song& x) const
{
 return std::find(list.begin(), list.end(), x) != list.end();
}