デストラクタについて

質問

#include <iostream>
#include <string>

using namespace std;

#define NUM 2 //登録人数

class Person{
char *name;
int *age;
char *hobby;
public:
Person()
{
name = new char;
age = new int;
hobby= new char;
}
void Set(char *n,int a,char *h)
{
name=n;
*age=a;
hobby=h;
}
char *Get_name(void)
{
return name;
}
int Get_age(void)
{
return *age;
}
char *Get_hobby(void)
{
return hobby;
}
~Person()
{
cout<<name<<"のデストラクタ\n";
delete [] name;
delete age;
delete [] hobby;
}
};
int main(void)
{
Person *p;
int i;

p=new Person[NUM];

p[0].Set("永嶋",21,"映画鑑賞");
p[1].Set("平林",54,"車");

for(i=0;i<NUM;i++){
cout<<"\n名前:"<<p[i].Get_name();
cout<<"\n年齢:"<<p[i].Get_age();
cout<<"\n趣味:"<<p[i].Get_hobby()<<"\n";
}

delete [] p;

return 0;
}
というプログラムを作成したのですが
デストラクタの3つのdeleteがおかしいようなのですが
どのような部分が問題となっているのでしょうか？
回答・アドバイス宜しくお願いいたします。

回答No.5

kmb01 · ベストアンサー

No2補足の回答です。

>Setのメソッドの中にある二つのif文は何のために有るのでしょうか？
>
>初心者が言うのは生意気ですが
>デストラクタの部分でdeleteすれば良いのではないのでしょうか？
同じオブジェクトに対して複数回Setが呼び出されたときに、
以前に確保したname,hobby領域を開放して、新しいname,hobbyの領域を確保しています。
これを行わずいきなりname=new char[strlen(n)+1];とやると、
nameが上書きされるため古いnameが開放できなくなります。

同じオブジェクトに対してSetは絶対に1回しか呼ばないならばこの処理は不要ですが、
そういった条件を設けることはあまり望ましくないと思います。

お礼

やっと意味が分りました。

再度回答ありがとうございました。

回答No.4

KamoPlat

眠い頭で回答してしまったのでちょっと日本語と用語の使い方がおかしいですね。申し訳ありませんでした。
もしコンストラクタのnewが変数の型の初期値で初期化したいなら、
age = int();
のようにするか、初期化リストを使うと良いですよ。
Person()
:name (NULL) , age (0) , hobby (NULL)
{

}
それとも、 *Get_hobby()とかをSet()するまえに呼び出したときの保険なら、
Get()の方で判定して
return "";
としてやるのもいいかも。ただし、const char*ですね、コレは。
そうでなければ、毎回きちんとSet()が呼び出されるたびにnew/deleteして
メモリ管理しなければなりません。(他の方の仰っているstringもそれを自動で
やってくれるものです)

お礼

アドバイス有難うございます。

いろいろ勉強できてためになります。

回答No.3

hidebu-

まずコンストラクタの

Person()
{
name = new char;
age = new int;
hobby= new char;
}

でヒープ領域にメモリ確保している意図がわかりません。

この処理ロジックだと、別にヒープにメモリ確保する必要なしかと。

-------------------------------------

#include <iostream>
#include <string>

using namespace std;

#define NUM 2 //登録人数

class Person{

char *name;
int age;
char *hobby;

public:

Person()
{
}

void Set(char *n,int a,char *h)
{
name=n;
age=a;
hobby=h;
}
char *Get_name(void)
{
return name;
}

int Get_age(void)
{
return age;
}

char *Get_hobby(void)
{
return hobby;
}

~Person()
{
cout<<name<<"のデストラクタ\n";
}

};

int main(void)
{

Person *p;
int i;

p=new Person[NUM];

p[0].Set("永嶋",21,"映画鑑賞");
p[1].Set("平林",54,"車");

for(i=0;i<NUM;i++){
cout<<"\n名前:"<<p[i].Get_name();
cout<<"\n年齢:"<<p[i].Get_age();
cout<<"\n趣味:"<<p[i].Get_hobby()<<"\n";
}

delete [] p;

return 0;

}

-------------------------------------

といった感じですかね。

よくわからないうちにむやみに多用するとリークおこしまくりますよ＾＾；

お礼

自分でも必要ないとは思っていたのですが
正確には私が書いたように白とは書いてないのですが
メソッドの中でnew演算子を使う事が
条件だったのであのようになってしまいました。
今後気をつけます。

回答有難うございました。

回答No.2

kmb01

char*型の代入はポインタ値がコピーされるだけで、その指す先がコピーされるわけではありません。
関数呼び出しでも文字列そのものが渡されるわけではなく先頭アドレスが渡されます。

上の例だと
p[0].Set("永嶋",21,"映画鑑賞");
としている部分では、メモリ上のどこかに永嶋、映画鑑賞というデータが書き込まれた領域が作成されていて、
その先頭アドレスが例えば100,200だとすると、関数呼び出し部分は
p[0].Set(100,21,200);
と呼び出されることになります。
呼び出されるSet内のname=n;はname=100;ということで、
nameに入っていたコンストラクタでnewした領域のアドレスは失われます。
そしてデストラクタでdeleteする領域がnewで確保した領域でないためエラーとなります。
また、name = new char;は1文字分しか領域を確保していないという間違いもあります。

解決案はstring型を使うか、以下のようにSet時に領域を確保する方法があります。
class Person{
char *name;
int age;//ポインタにする意味がないと思う
char *hobby;
public:
Person()
{
name = NULL;
hobby= NULL;
}
void Set(char *n,int a,char *h)
{
if (name) delete[] name;
name=new char[strlen(n)+1];
strcpy(name, n);
age=a;
if (hobby) {delete[] hobby;}
hobby=new char[strlen(h)+1];
strcpy(hobby, h);
}

お礼

回答有難うございました。

Setのメソッドの中にある二つのif文は何のために有るのでしょうか？

初心者が言うのは生意気ですが
デストラクタの部分でdeleteすれば良いのではないのでしょうか？

見当はずれな事を聞いているかもしれませんが
良い機会なので教えていただければ幸いです。
宜しくお願いします。

回答No.1

KamoPlat

いろいろまずいと思います。
まず、コンストラクタで
new char;
などとしているのに、デストラクタで
delete [] name;
などとなっています。
これはまずいです。
それと、
Set()でnに指定されるのはnewされた文字列へのポインタではなく
ヒープのアドレスです。
new / deleteとnew [] / delete [] は対応させないと。

お礼

回答有難うございます。