2

レガシーファイルを表すクラスを実装しています。ファイルはバイナリであり、広範囲にわたる解析が必要です。ファイルを解析するためのコードはおそらく数千行あります。クラスには2つのコンストラクターがあります。

class CrmxFile {

public:
    CrmxFile(std::wstring filename);
    CrmxFile(std::ifstream& file);

...
}

オブジェクトはファイルの内容なしでは意味をなさないため、オブジェクトの作成中にすべての解析を実行する必要があります。私は大規模なコンストラクター（streamパラメーターを持つコンストラクター）を実装しようとしていましたが、私の同僚は、非常に大規模なコンストラクターを使用するのは良い習慣ではないと主張しています。代わりに、解析を実行するプライベートメソッドを作成し、ストリームが読み取り可能であることを確認した後、コンストラクターからこのメソッドを呼び出す必要があります。おそらく、ファイルヘッダーを読み取って、ストリームに正しいデータが含まれていることを検証します。

ガイドライン/ルール/規約などはありますか？そのような状況を支配するのですか？基本的に（擬似コードで）、2つのアプローチのどちらが優れているか、

長いコンストラクター：

CrmxFile::CrmxFile(std::ifstream& file) {
    if (file is not readable) {
        throw new CmrxException(read_error);
    }

    CmrxHeader header(file);
    if(!header.isValid()) {
        throw new CmrxException(invalid_file);
    }

    //now do all the reading/parsing of the file
    //constructor may end up being a thousand lines of code
    ...
}

または短いコンストラクターとヘルパーメソッド：

class CrmxFile {

public:
    CrmxFile(std::wstring filename);
    CrmxFile(std::ifstream& file);

private:
    ParseFile(std::ifstream& file);

...
}

CrmxFile::CrmxFile(std::ifstream& file) {
    if (file is not readable) {
        throw new CrmxException(read_error);
    }

    CrmxHeader header(file);
    if(!header.isValid()) {
        throw new CrmxException(invalid_file);
    }

    ParseFile(file);
}

void CrmxFile::ParseFile(std::ifstream& file) {
    //do all the reading/parsing of the file here
    ...
}
4

4 に答える 4

5

ヘルパーはここで実行可能のようです。

ただし、非公開にすることはしません。代わりに、詳細またはパーサーの名前空間にある可能な限り小さい関数に分割して、各メソッドをテストできるようにします。istreamとにかくストリームを使用するので、これは簡単です。代わりにを受け入れるようにメソッドを変更するifstreamと、テストへの入力をプレーン文字列にして、メソッド引数として使用するistringstreamにフィードできます。私を信じてください、あなたはこれをテストしたいと思うでしょう、あなたが今すでに言っているように、それは数百行になるでしょう、あなたが最初からそれを正しくするチャンスはありません。

実際のヘルパーメソッドは、単純に小さなメソッドを組み合わせたり、使用しているメソッドの量に応じて、他のメソッドを組み合わせたりする小さなメソッドを組み合わせます。

于 2013-01-04T12:39:25.377 に答える
0

複数のコンストラクターがある場合は、初期化を行うためのプライベート メソッドを用意することをお勧めします。繰り返しコードを保存します (したがって、保守性が向上し、変更が行われた場合にエラーが発生しにくくなります)。

コンストラクターが 1 つだけの場合は、必要なだけ複雑にします。

于 2013-01-04T12:37:27.937 に答える
0

他の用途やオブジェクト内の複数のコンストラクターのためにプライベートヘルパーメソッドを共有したくない場合を除き、これは本当に重要ではないと思います。

コンストラクター内で解析するアプローチは、私には問題があるように思えます。これは、構築と初期化の役割が混在しています。解析をパブリック インターフェイスとして公開する場合、コンストラクターはオブジェクトの状態を初期化されていない状態に設定するだけで、オブジェクトをクライアントに使用できないものとしてマークします。または、ここで遅延評価を使用してシングルトン パターンを適用してみてください。まだ解析されていない場合getInstanceにプライベートを呼び出すメソッドを実装します。ユーザーの観点からは透過的です (最初のアクセスのオーバーヘッドを除く)。parse

于 2013-01-04T12:44:47.823 に答える
0

「コンストラクターではやらないでください、ピリオド」と言います。

コンストラクターは奇妙な獣であり、構築中の失敗は、例外をスローすることによってのみ発生する可能性があります。また、壊れた/無効なファイル、または解析エラーは (私の意見では) 投げるにはあまりにも一般的です。

したがって、より簡単な道をたどってください。

オブジェクトを生成するファクトリ関数またはファクトリ オブジェクトを記述します。解析状態を維持し、オブジェクトを解析する無数の関数 (必要に応じて 1 つの大きな関数) を備えています。次に、解析が完了すると、オブジェクトが返されます。そのオブジェクトには、ディスクからの解析に関連するコード (または状態) の山はありません。

これは、代わりに

try {
  MyObject foo("file.blah");
  // ...
} catch (MyObject::ParseError err) {
  // ...
}

代わりに次のようにします。

MyObjectParser parser;
parser.Load("file.blah");
if (parser.Error()) {
  // ...
} else {
  MyObject foo = parser.Get();
  // ...
}

MyObject存在する唯一のものMyObjectは、すでに読み込まれている商品でなければならないという状況があります。

于 2013-01-04T13:45:52.487 に答える