Overloading << operator

Question

I have to make for a project an Album class which uses a Song class. I seem to have problem with overloading the << operator for the Album class. So this is the album class

---

const int Empty=-1
class Album
{
  Song* songs;
  char* name;
  int top=Empty;
public:
  Album ();
  Album (const char*);
  ~Album();
  friend std::ostream& operator<<(std::ostream&,Album&);
  void addSong(Song&);
};

I'm worried that there might be a mistake with my functions so far but I can't find it. the program starts but crashes ,any ideas why ? I don't think the problem is in songs[top]=p, because i have redefined the assignment operator for the Song class.

---

void Album::addSong(Song& p)
{
  ++top;
  songs[top]=p;

}
Album::~Album()
{
  delete[] name;
}
std::ostream& operator<<(std::ostream& os, Album &p)
{
  os<<"Album name:"<<p.name<<std::endl;
  os<<"Songs in the album:"<<std::endl;
  for(int i=0;i<=p.top;i++)
  std::cout<<p.songs[i];
  return os;
}
Album::Album()
{
  top=Empty;
  name=new char [1];
  name[0]='\0';
}
Album::Album(const char*p)
{
  delete [] name;
  int len1=strlen(p);
  name=new char [len1+1];
  strcpy(name,p);
}

Answer 1

Your overloaded operator is a little broken (as the comments pointed out) but is unlikely to crash your program. There are many problems with this code that will crash it.

To start with, songs is not initialized, which means that your songs[top]=p; are actually storing the value of p to somewhere in the memory pointed to by stack garbage. That will likely crash your code. You are also not actually allocating storage space to store the songs you added. Note that even if you initially allocated space to store one Song, you can't just increment top and store the next Song; your addSong will have to dynamically grow the allocated memory. Now, if you have a cap on the number of Songs an album can store, you can use the songs=new Song[MAX_SONGS] approach suggested (and make addSong fail if the album hit its max capacity), but it's far better to use vectors and strings instead of pointers and raw news and deletes if possible, and leave the memory management headache to the standard library. (You also didn't provide copy constructors and overloaded assignment operators; you would need both when your class does this sort of memory allocation, to prevent memory leaks and double deletion.)

Note that your constructor that takes a string is also broken; you haven't allocated any memory in name at that point so there's nothing to delete.

---

Edit: To elaborate on my comment below, if you have to be able to handle arbitrary numbers of songs and you have to use new and delete, then your addSong will have to do something like this (pseudocode):

void Album::addSong(Song& p)
{
  new_size = size + 1;
  new_songs = new Songs[new_size];
  for( i = 0; i < size; i++)
     new_songs[i] = songs[i];
  new_songs[size] = p;
  delete [] songs;
  songs = new_songs;
  size = new_size;
}

Your destructor will need to delete [] songs;. Your constructor should set size to 0 and songs to the null pointer (it's legal to use delete on the null pointer; it has no effect).

You should also overload the assignment operator (which should first delete your own name and songs, allocate enough memory to hold the other album's name and songs, and copy them over) and the copy constructor (which should allocate enough memory to hold the other album's name and songs and then copy them over).

Answer 2

You don't want:

delete [] name;

in your constructor, and want:

return os;

at the end of operator<<().

Also Songs isn't used.

Answer 3

One source of crashes:

Album::Album(const char*p)
{
    delete [] name;   // <--- right here

This will take uninitialized memory and apply delete it (it is essentially UB).

Edit (as others have pointed out): songs needs to be initialized (similar to name) before you place stuff into it.

More, to avoid a memory leak, also delete[] songs in Album::~Album.

Answer 4

You haven't allocated any space for songs.