Cpp string::copy function adds additional characters

Question

I use string::copy function in my copy constructor(deep copy) and when I use it, it appends some not meaningful characters to my string. Here is my main function:

#include <iostream>
#include "SimpleMusicAlbum.h"

int main(){


    MusicAlbum msc("a7x","Seize the day",2005);

    cout << msc.getMusicAlbumArtist()   <<endl;
    cout << msc.getMusicAlbumTitle()    <<endl;
    cout << msc.getMusicAlbumYear()     <<endl;

    MusicAlbum msc2(msc);
    cout << msc2.getMusicAlbumArtist()   <<endl;
    cout << msc2.getMusicAlbumTitle()    <<endl;
    cout << msc2.getMusicAlbumYear()     <<endl;
    //MusicAlbum msc3(msc);

    return 0;
}

a strange point is that when I write a7x instead of Avenged sevenfold to title, it does not append characters.

Here is the header SimpleMusicAlbum.h:

#ifndef __SIMPLE_MUSIC_ALBUM_H
#define __SIMPLE_MUSIC_ALBUM_H
#include <string>
using namespace std;
class MusicAlbum {
 public:
    MusicAlbum(const string maArtist = "",
    const string maTitle = "",
    const int maYear = 0);

    ~MusicAlbum();
    MusicAlbum(const MusicAlbum &maToCopy);

    void operator=(const MusicAlbum &right);

    string getMusicAlbumArtist();
    string getMusicAlbumTitle();
    int getMusicAlbumYear();
 private:
    string artist;
    string title;
    int year;
};
#endif

Here is SimpleMusicAlbum.cpp:

MusicAlbum::MusicAlbum(const string maArtist,
            const string maTitle,
            const int maYear){
    artist = maArtist;
    title = maTitle;
    year = maYear;

}
//the problem is here
MusicAlbum::MusicAlbum(const MusicAlbum &maToCopy){
    char artistTemp[maToCopy.artist.size()] ;
    char titleTemp[maToCopy.title.size()];

    cout << maToCopy.artist.size() << endl;

    maToCopy.artist.copy(artistTemp, maToCopy.artist.size(), 0);
    artist = artistTemp;
    maToCopy.title.copy(titleTemp,maToCopy.title.size(),0);
    title = titleTemp;
    this->year = maToCopy.year;
}

//same problem occurs here
void MusicAlbum::operator=(const MusicAlbum &right){
    char artistTemp[right.artist.size()];
    char titleTemp[right.title.size()];
    right.artist.copy(artistTemp, right.artist.size(), 0);
    artist = artistTemp;
    right.title.copy(titleTemp,right.title.size(),0);
    title = titleTemp;
    this->year = right.year;
}
//destructor
MusicAlbum::~MusicAlbum(){
    // no allocation, no destruction.
}
//methods
string MusicAlbum::getMusicAlbumArtist() {
    return artist;
}

string MusicAlbum::getMusicAlbumTitle(){
    return title;
}

int MusicAlbum::getMusicAlbumYear(){
    return year;
}

Answer 1

Standard library containers have their own copy and assignment functions. There's no need to manually copy them.

 MusicAlbum::MusicAlbum(const MusicAlbum    &maToCopy)
 {
     artist  = maToCopy.getMusicAlbumArtist();
     title = maToCopy.getMusicAlbumTitle();
     year = maToCopy.getMusicAlbumYear();   
 }

Here is a working example.

 #include<string>
 #include<iostream>

 using namespace std;

 class MusicAlbum
 {
     public:
      MusicAlbum(string Artist, string Title, int Year):artist(Artist),
      title(Title),
      year(Year)
      {}
      string getTitle(){ return title; }
      string getArtist() { return artist; }
      int getYear() { return year; }
 private:
     string artist;
     string title;
     int year;  
 };

 int main()
 {
     MusicAlbum a{"a7x","albumname",2007};
         MusicAlbum b = a;
         cout << b.getArtist() << " " << b.getTitle() << " " <<   b.getYear() << endl;
 }

Answer 2

Let's break down the MusicAlbum class to only its member variables:

class MusicAlbum 
{
    //...
    string artist;
    string title;
    int year;
};

Given these are the member variables, making copies of MusicAlbum is perfectly safe to do if you have the compiler do the copies. Thus there is no need to supply a user-defined copy constructor and assignment operator. A std::string has correct copy semantics out of the box, and an int is obviously safely copyable. So the fix is simple:

remove those functions from your class.

If however MusicAlbum contained pointers to dynamically allocated memory, or resources that need to be handled, or some other aspect that the default compiler implementation of the copy constructor and assignment operator will not correctly provide, then yes, you would write a user-defined copy constructor / assignment operator.

---

So what can happen if you provide the copy constructor and assignment operator functions when you don't need to provide them?

The only outcomes are:

1. You wrote these functions correctly.
2. You wrote these functions incorrectly.

You would think that option 1. would be ok. Maybe, but the downside is that you may have written your version inefficiently (more than likely you did). The compiler's default copy/assignment will always work correctly, and would have been written efficiently.

As for option 2., this is what is happening to your code. Rather than debugging, simply remove those functions. If you were to debug and get these functions working correctly, you then fall right back into item 1 on the list. So you really didn't gain anything, except a few minutes or hours writing and debugging functions you didn't need to write in the first place.

So don't waste your time writing functions that the compiler already provides for you. Given the member variables in your class, the compiler will always get the copies done correctly.