質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

ただいまの
回答率

90.38%

  • C++

    4653questions

    C++はC言語をもとにしてつくられた最もよく使われるマルチパラダイムプログラミング言語の1つです。オブジェクト指向、ジェネリック、命令型など広く対応しており、多目的に使用されています。

C++ メンバー変数に自身のクラスを使う また、そのNULLチェック

解決済

回答 4

投稿 編集

  • 評価
  • クリップ 0
  • VIEW 760

tf2014

score 51

C++勉強中のものです。クラスについて、うまくわからないところがあるので質問させて下さい。

(以下のコードはC#ので書かれたものを、C++実現しようとして、書いたのでおそらく沢山ご指摘していただける部分があると思います。改善するべきところが多くあると思うので質問内容の外のご指摘も歓迎いたします。)

ノードオブジェクトに次のノード メンバーがあり、ShowList関数では自分の値をプリントし、次のノードを設定し、次のNodeがある限り、ループするというものです。 

Nodeクラスに、自身のNode型のメンバー next を作りたいのですが、いろいろ検索した結果、ポインタかレファレンスを使わなくては実現できないと見つけ、レファレンスはその存在が保証されなければ壊れるので、避けるべきだと読みました。その結果、ポインタ、Node* next に決定しました。 

質問1)一般にポインターはなるべく避けるべきと聞きますが、ポインターなしのより良い方法がありますか?

質問2) while(node != NULL) 次のノードが存在する限り、ループするというコードですが、これがうまく機能していないようです。つまり、threeまでしか、next を設定していないので、1, 2, 3, 4 と出力されると期待しているのですが、1, 2, 3, 4, 25, Segmentation fault:11 と出力されます。
何を改善するべきなのでしょう?

#include <iostream>

class Node{
public:
  Node(int v);
  int value;
  Node* next;
};

Node::Node(int v){
  this->value = v;
}

void showList(Node* node){

    while(node != NULL){
    std::cout << node->value << "\n";
    node = node->next;
    }
}

int main(){
  Node one(1);
  Node two(2);
  Node three(3);
  Node four(4);

  one.next = &two;
  two.next = &three;
  three.next = &four;

  showList(&one);

  return 0;
}

皆さん、ご回答ありがとうございました。色々、各解答、とても勉強になりました。一番勉強になったのは初期化の大切さです。以下、皆さんの、回答をもとに、試した、自分なりの答えです。

#include <iostream>

struct Node{
  Node(int val, Node* n);  //初期化強制。ディフォルト無し。
  ~Node();
  int value;
  Node* next_address;
};

Node::Node(int val, Node* next_addr){
  this->value = val;
  this->next_address = next_addr;
}

void showList(Node* node){
    while(node != nullptr){
    std::cout << node->value << "\n";
    node = node->next_address;
    }
}


int main(){

  Node* one = new Node(1, nullptr);
  Node* two = new Node(2, nullptr);
  Node* three = new Node(3, nullptr);
  Node* four = new Node(4, nullptr);
  Node* five = new Node(5, nullptr);

  one->next_address = two;
  two->next_address = three;
  three->next_address = four;
  four->next_address = five;

  showList(one);


  // 借りたら、返す
  delete one;
  delete two;
  delete three;
  delete four;
  delete five;

  return 0;
}
  • 気になる質問をクリップする

    クリップした質問は、後からいつでもマイページで確認できます。

    またクリップした質問に回答があった際、通知やメールを受け取ることができます。

    クリップを取り消します

  • 良い質問の評価を上げる

    以下のような質問は評価を上げましょう

    • 質問内容が明確
    • 自分も答えを知りたい
    • 質問者以外のユーザにも役立つ

    評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。

    質問の評価を上げたことを取り消します

  • 評価を下げられる数の上限に達しました

    評価を下げることができません

    • 1日5回まで評価を下げられます
    • 1日に1ユーザに対して2回まで評価を下げられます

    質問の評価を下げる

    teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。

    • プログラミングに関係のない質問
    • やってほしいことだけを記載した丸投げの質問
    • 問題・課題が含まれていない質問
    • 意図的に内容が抹消された質問
    • 広告と受け取られるような投稿

    評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。

    質問の評価を下げたことを取り消します

    この機能は開放されていません

    評価を下げる条件を満たしてません

    評価を下げる理由を選択してください

    詳細な説明はこちら

    上記に当てはまらず、質問内容が明確になっていない質問には「情報の追加・修正依頼」機能からコメントをしてください。

    質問の評価を下げる機能の利用条件

    この機能を利用するためには、以下の事項を行う必要があります。

回答 4

+6

こんにちは。

以下のコードはC#ので書かれたものを、C++実現しようとして

なるほど。C#とC++の相違点は数多いですが、個人的に最大の相違の1つはメモリ管理方式と考えています。
C++は「借りたら返す」です。返した後は使ってはいけません。間違って使ったら運が良ければ例外ですが、何事も無く動いて後で痛い目に合うケースもあります。

C#は「使い終わったら自動的に回収」です。使い終わらない限りいつまでも回収されず、使い終わっても直ぐには回収されません。そして、使い終わったら(null設定したら)使いようがなく、間違って使うと必ず例外が発生します。
一長一短ですが、C#のメモリ管理が楽な点はC#の生産性の高さに大きく貢献しています。

ポインタかレファレンスを使わなくては実現できないと見つけ、レファレンスはその存在が保証されなければ壊れるので、避けるべきだと読みました。その結果、ポインタ、Node* next に決定しました。 

良い発想と思います。
基本的に参照はポインタの機能制限版です。ポインタの方が自由度が高いのですが、設計的に使わない機能は使えないように制限した方がバグは減ります。(間違って使わないので)
なのでポインタを使う必要がない時は参照を使った方が安全です。

質問1)一般にポインターはなるべく避けるべきと聞きますが、ポインターなしのより良い方法がありますか?

今回のコードでは生ポインタでも問題ないと思います。
「生ポインタはなるべくさけるべき」はメモリ・リークの可能性が高いからです。ポインタの指す領域をnew等で借りたら必ず返す必要があります。あちこちで借りまくるのできちんと返す設計は結構たいへんなため、バグの温床になりやすいです。なので「生ポインタはなるべくさけるべき」なのです。今回のコードはローカル変数を指しています。通常のローカル変数はその関数から抜ける時に自動的に開放されますからメモリ・リークの可能性はありません。

質問内容の外のご指摘も大歓迎いたします。

C++/C#共通の話で申し訳ないのですが、下記のインデントを嫌う人は多いと思います。

    while(node != NULL){
    std::cout << node->value << "\n";
    node = node->next;
    }

このwhile文の中にもう一つwhile文を入れて二重ループにすることを想定してみて下さい。ご理解頂けると思います。

また、Node* next;はprivateにした方が望ましいと思います。Node領域管理方式はNodeの内部実装になると思います。その方式を変更した時にNodeクラスを使っている外部のコードの修正が必要になるのは避けたいケースが多いです。privateにしておけば、外部のコードが「間違って」使う可能性はなくなりますから、管理方式を変更しても外部のコードへの影響はありえません。

なお、valueは微妙です。多少無理してもprivateにすべきとの意見もよく見かけますが、私はpublicも有りと思います。
こちらは有る種の構造体やコンテナ的な使い方と思います。例えば、valueの型を変更した場合、Nodeクラスを使っているコードも変更が必要になるケースが多いでしょう。この場合、手間をかけてprivate化するメリットが乏しいです。

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2019/02/11 14:31

    とても詳しい説明ありがとうございます。熟読し吟味、実験、失敗を繰り返したい思います。特に、借りたら返すの発想は面白そうです。qiitaの記事も飛ばし読みしてみましたが、時間をかけて、読んでいきます。

    キャンセル

  • 2019/02/12 14:24

    コメントを読み直して思ったんですが、private, public / class , struct も色々考えさせられる部分なんですよね。いろいろ、読み漁ってみるとクラスと構造体の違いは、ディフォルトがパブリックかプライベートかが違うだけで後は同じと、かいてあり、そうすると、全て構造体にしたほうがすっきりするんじゃないかと考えたり、どの状況でどちらを使うべきなのか、迷います。 これは、これ自体でいろんなコメントを頂けると思うので、初心者なりの考えをまとめて別に質問させていただきます。

    キャンセル

  • 2019/02/12 14:31

    メモリーリーク、ポインターの失敗も勉強の一環として、大失敗してみたいですけど。 グローバルでポインタをつくり、実験したいと思います。

    キャンセル

  • 2019/02/12 16:11

    特に最近のC++は凄い発想の塊ですから得るものは非常に多いです。頑張って下さい。

    キャンセル

checkベストアンサー

+3

コンストラクタが明らかに足りません。

Node::Node(int v){
  this->value = v;
}


これだけではnextに何も入っていない状態になってしまうため、

Node::Node(int v){
  this->value = v;
  next = NULL;
}

後はリストの場合はデストラクタが必ず必要です。
ないとプログラムが落ちますよ。
リストならstdの中にvectorやlistがあるのでそっちを
利用するのも手ですね。

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2019/02/13 12:42

    今回の質問者さんのケースでは、動的確保をしておらずローカル変数のアドレスを代入しているだけなのでデストラクタは不要ですね。実際に落ちてはいますが、今回はデストラクタとは無関係なので誤解を生む可能性はあるかなと見えました。
    それでもリストに追加する要素がローカル変数の内容のみというのはあまり現実的ではないので、 (C# では要らない) メモリ解放処理が C++ では必須という意味でデストラクタは必要ですよね。 (そのときは逆にローカル変数のアドレスをリストに追加されると困りますが。)

    キャンセル

  • 2019/02/13 12:49

    えっと,それはつまり【現象として「実際に落ちてる」のであれば,回答の記述としてはその原因側の話は正しくなくても別に良い】というようなスタンス(?)ということですか?

    キャンセル

  • 2019/02/13 13:43

    > (そのときは逆にローカル変数のアドレスをリストに追加されると困りますが。)
    nodeに好き勝手なアドレスが入れられ得るのに,デストラクタに解放処理を書く,というのは何だかおかしな話に感じてしまいます.
    今回のケースで相手にしている実装は「publicなnextの値をいじくるのはNode型の利用側」という形ですから,nextに指定される領域を{用意すること,必要ならば解放すること}は全てNode型を利用する側の責任ではないかと思うのですが,どうなんでしょう?

    キャンセル

+3

// C++では"初期値は0"ルールが無い。nextを明示的に初期化すべし。
Node::Node(int v) : value(v), next(nullptr) {}

void showList(Node* node){
  for ( ; node != nullptr ; node = node->next ) {
    std::cout << node->value << "\n";
  }
}

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2019/02/11 10:32

    なるほど!「初期値は0"ルールが無い」勉強になります。 ありがとうございます。実験してみます。

    キャンセル

  • 2019/02/12 14:11

    stdioさんのコメントを見た後、色々読みあさって、考えてみて、epistemeさんのコメントを読み直したら、Node::Node(int v) : value(v), next(nullptr) {}の便利さがわかりました。正直、昨日は意味をよく理解できていませんでした。「初期化が命」のため、nullptrに初期化。つまり、Node(int v, Node n)というコンストラクターを定義しなくても初期化はできるという便利ものですね。

    キャンセル

+2

#include <iostream>
#include <memory>
#include <functional>

class Node{
  int value;
  std::unique_ptr<Node> next;
public:
  Node(int v, Node* next_node = nullptr);
  ~Node();
  void eachValue(std::function<bool(int)> action);
};

Node::Node(int v, Node* next_node) : value(v), next(next_node){}
Node::~Node(){
  std::cout << "destruct(" << value << ")" << std::endl;
}
void Node::eachValue(std::function<bool(int)> action){
  if(action(value) && next)
    next->eachValue(action);
}

void showList(Node& node){
  node.eachValue([](int val){
    std::cout << val << std::endl;
    return true;
  });
}

int main(){
  Node one(1, new Node(2, new Node(3, new Node(4))));

  showList(one);

  return 0;
}


やってみたこと

  • ポインタの代わりにスマートポインタ(unique_ptr)を使う
  • valueおよびnextをprivateへ移行し、それに伴いeachValueやらコンストラクタやらを弄る
  • 子ノードのデストラクタが呼び出される事を証明するためにデストラクタを定義

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2019/02/12 14:38

    私が気にしてた事を省略すると、

    リストはよく列車に例えられる。そして車両の一つ一つが今回でいう「next」。
    列車は連結部を切り離すと、そこより後ろの車両は同然動力を失い、停車してしまう。
    もしそれがプログラムの中で起こると、切り離した車両から後ろ(ここでいうnext->next)は行方不明になってしまって永遠(PCが起動している間)にメモリの海を彷徨うって訳。

    キャンセル

  • 2019/02/13 10:05 編集

    行方不明にはなりますが、
    > 永遠(PCが起動している間)
    は言いすぎですね。

    現代的なOSの場合、プロセス終了時に割り当てたメモリは全解放されるシステムが一般的です。
    起動しっぱなしのプログラムや特殊な環境でもない限り、そこまで神経質になる必要があるかは疑問です。

    まぁ行儀悪いと言われればそれまでですが

    キャンセル

  • 2019/02/13 10:12

    そうですね。流石に言いすぎですね...
    しかし、ゲーム開発経験があるとやたらと気になるのですよ。
    ゲームにおいては、「プロセス終了時=ゲームのプレイをやめた時」なので、しっかりと解放しないといけなかったりします。

    キャンセル

15分調べてもわからないことは、teratailで質問しよう!

  • ただいまの回答率 90.38%
  • 質問をまとめることで、思考を整理して素早く解決
  • テンプレート機能で、簡単に質問をまとめられる

同じタグがついた質問を見る

  • C++

    4653questions

    C++はC言語をもとにしてつくられた最もよく使われるマルチパラダイムプログラミング言語の1つです。オブジェクト指向、ジェネリック、命令型など広く対応しており、多目的に使用されています。