TemplateMethodパターンでのクラス設計について

Question

前提・実現したいこと

ファイル読み込み→変換→ファイル出力を行うような処理をTempleteMethodパターンで作成しようとしています。
以下の様な処理で考えているのですが、何点か気になる点があり、クラス設計に問題があるのか悩んでいます。
問題点をご指摘いただけないでしょうか。

前提

• Dto1,Dto2やConvertDto1,ConvertDto2の項目は全く違うもので抽象化はできません

気になる点

1. パラメータがobject型なので具象クラスを作成する人間が毎回object型を適切な型に変換し、後続の処理を行う必要がある
2. Dtoが抽象化できないのでAbstractClassのメソッドのパラメータ・返り値、メンバ変数の定義が
   軒並みobject型になってしまい、変数やメソッドの用途がイマイチ分かりづらい
3. そもそもTemplateMethodパターンのサンプルで、抽象メソッドに返り値を持たせる
   ようなものをあまり見かけません、ということはTemplateMethodパターンを使うこと自体が間違っている？

処理

    public abstract class AbstractClass
    {
        protected abstract IEnumerable<object> ReadFile();

        protected abstract IEnumerable<object> Convert(IEnumerable<object> readDtos);

        protected abstract void WriteFile(IEnumerable<object> convertDtos);

        private IEnumerable<object> _readDtos;

        private IEnumerable<object> _convertDtos;

        public void Run()
        {
            // ファイルを読み込みDtoに設定
            this._readDtos = this.ReadFile();

            // 読み込んだデータを変換
            this._convertDtos = this.Convert(this._readDtos);

            // ファイル出力
            this.WriteFile(this._convertDtos);
        }
    }

    public class ConcreteClass1 : AbstractClass
    {
        protected override IEnumerable<object> ReadFile()
        {
            // ファイル読み込み

            // 読み込み内容をDtoの配列で返却

            return IEnumerable<dto1>
        }

        protected override IEnumerable<object> Convert(IEnumerable<object> readDtos)
        {
            // パラメータがobjectなのでDto1に変換しなければならない
            var dtos = (IEnumerable<Dto1>)readDtos;

            // readDto→convertDto変換処理

            return IEnumerable<convertDto1>;
        }

        protected override void WriteFile(IEnumerable<object> convertDtos)
        {
            // パラメータがobjectなのでConvertDto1に変換しなければならない
            var dtos = (IEnumerable<ConvertDto1>)convertDtos;

            // ファイル書き込み処理
        }
    }

    public class ConcreteClass2 : AbstractClass
    {
        protected override IEnumerable<object> ReadFile()
        {
            return IEnumerable<dto2>;
        }

        protected override IEnumerable<object> Convert(IEnumerable<object> readDtos)
        {
            var dtos = (IEnumerable<Dto2>)readDtos;

            return IEnumerable<convertDto2>;
        }

        protected override void WriteFile(IEnumerable<object> convertDtos)
        {
            var dtos = (IEnumerable<ConvertDto2>)convertDtos;
        }
    }

Comments

[Zuishin]

何がしたいのかよくわかりません。ここにコードで表されている処理を具体的に書き換えることはできますが、それが実際にやりたいことに当てはめられるかどうかわかりません。

具体的にファイルをどう変換したいのか(エンコーディングを変えるのか文字列を読み取って置換するのかなど)書いた方がいいと思います。ただ「変換したい」というだけなら何百種類も方法があります。

[Zuishin]

また、Read, Convert, Write の三種類のメソッドに分けることが良いのかどうかもここからは読み取れません。object としか言えないなら分ける必要はありません。メソッドは一つで済むでしょう。Stream から Read し、Stream に Write するメソッドはわざわざ実装しなくても既にあるからです。

[退会済みユーザー]

具象クラス側の処理内容も踏まえてじゃないと
正確なアドバイスができない、ということですね。

質問の意図としては、もう少し大まかな感じで考えていました。

Read, Convert, Writeには、何かしら具象クラスに記述しないといけない
個別の処理があり、ReadではConvertに渡すデータを取得している。
という前提で、その様な処理の場合TemplateMethodパターンを使うべきなのか
他に何か良いパターンがあるのか
また、TemplateMethodを使いもっと分かりやすく記述する方法があるのか
という事をご教授いただきたいと考えていました。

[Zuishin]

踏まえてというか、三種類のメソッドを作るメリットを説明できますか？
無いように見えます。
ならばテンプレート自体が間違っていますので、別の方法をとるべきです。

[Zuishin]

ここに書かれたコードだけ見ると、全く無意味なことをしているようにしか見えません。無意味でない可能性を探るために具体的なやりたいことを聞いたのですが、無いならばやはり無意味です。

[退会済みユーザー]

必ず３種類のメソッドが必要か？と言われるとそうではないと思いますが
単純に大きな１メソッドで３メソッドの分の処理を記述するより
３メソッドに分けた方が処理が分かりやすくなるのでは？と思っています。

[Zuishin]

private でいいじゃないですか。

[Zuishin]

継承できないし外から使えないものは全部 private でいいんですよ。互換性がないということは継承できないということでしょう？
ここで継承させるべきものは Run ただ一つだけです。

そりゃジェネリックを使って形だけは無理やり継承させることはできますが、それは単なる継承のための継承で、無意味な作業です。手段と目的をはき違えています。

「何を継承させるか」が設計です。

[退会済みユーザー]

例えばReadの処理を考えると、イメージしていたのは具象クラスのReadメソッドではCsv、Excel、DB等から
データを読み込んで、Dtoに値を設定する事を考えていました。
この処理って出来ないことは無いと思いますが、AbstractClassにprivateで書かない方が良くないですか？

[Zuishin]

その場合は public メソッドのシリアライザを作るべきです。Convert は不要です。デコードした時に共通のデータが得られるはずです。いや、共通のデータにデコードすべきです。

[退会済みユーザー]

むむむ、シリアライザですか、、、ちょっとググってきます

Answer 1

AbstractClassをジェネリック化してみてはいかがでしょうか。DTO系の型とDTOConvert系の型を2つ受け取り、派生クラスを定義する際、型を指定して派生することで、特定のDTOからDTOConvertへ変換するためのRunメソッドを持ったConcrateClassを作ることができると思います。
また抽象化はできないそうですが、インターフェイスは使えるのではないかと思います。メンバーを持たないインターフェイス、”マーカーインターフェイス”というものです。

    public interface ITDto { }

    public interface ITDtoConvert { }

    public class DTO1 : ITDto { }

    public class DTO2 : ITDto { }

    public class ConvertDto1 : ITDtoConvert { }

    public class ConvertDto2 : ITDtoConvert { }

    public abstract class AbstractClass<TDto, TDtoConvert> where TDto : ITDto where TDtoConvert : ITDtoConvert
    {
        protected abstract IEnumerable<TDto> ReadFile();

        protected abstract IEnumerable<TDtoConvert> Convert(IEnumerable<TDto> readDtos);

        protected abstract void WriteFile(IEnumerable<TDtoConvert> convertDtos);

        public void Run()
        {
            // ファイルを読み込みDtoに設定
            IEnumerable<TDto> readDtos = ReadFile();

            // 読み込んだデータを変換
            IEnumerable<TDtoConvert> convertDtos = Convert(readDtos);

            // ファイル出力
            WriteFile(convertDtos);
        }
    }

    public class ConcrateClass1 : AbstractClass<DTO1, ConvertDto1>
    {
        protected override IEnumerable<ConvertDto1> Convert(IEnumerable<DTO1> readDtos)
        {
            throw new NotImplementedException();
        }

        protected override IEnumerable<DTO1> ReadFile()
        {
            throw new NotImplementedException();
        }

        protected override void WriteFile(IEnumerable<ConvertDto1> convertDtos)
        {
            throw new NotImplementedException();
        }
    }

    public class ConcrateClass2 : AbstractClass<DTO2, ConvertDto2>
    {
        protected override IEnumerable<ConvertDto2> Convert(IEnumerable<DTO2> readDtos)
        {
            throw new NotImplementedException();
        }

        protected override IEnumerable<DTO2> ReadFile()
        {
            throw new NotImplementedException();
        }

        protected override void WriteFile(IEnumerable<ConvertDto2> convertDtos)
        {
            throw new NotImplementedException();
        }
    }

Comments

[退会済みユーザー]

やってみました、なるほど、そんな方法があるんですね
型がobjectになるより全然わかりやすいですねー
”マーカーインターフェイス”参考になりました、ありがとうございます。

[Zuishin]

ジェネリックは私も真っ先に浮かびましたが、質問は「設計に問題は無いか」ということです。

この設計で問題ありませんか？

public ならまだ多少はわかります。protected で互換性の無い TDto を使う意味はありますか？
読みにくくなるだけではありませんか？

IEnumerable<TDto> の代わりに Stream や IEnumerable<Byte> ではいけませんか？

TDto にファイルからロードする機能を持たせるのではいけませんか？

互換性が無いのにわざわざこのテンプレートを使う意味は何でしょう？

[Zuishin]

返事が得られないのでもう少し詳しくかきます。

質問の設計では、Excel, CSV, DB それぞれの読み込みで互換性の無いデータを取り出し、それをコンバータで変換するようになります。

この回答はそれを踏襲し、機械的にジェネリックに置き換えています。すると少なくとも三種類の TDto が存在し、コンバータは六種類作らなければなりません。そうでなければ、object 型でなければならなかった理由が無いからです。

非常に悪い設計だと思うのですが、どうでしょうか？

[Gurz1019_MP]

Zuishin様、返信が遅くなり失礼致しました。ご指摘の通り、問題がないかという本題を失念して気になる点の1と2に対する回答のみをしてしまいました。
私自身このような設計を最近良くするのですが、正直なところ良し悪しがよくわかりません。今思えば変換元と先のDTOが一対一に対応しているなら、変換元に変換機能を持たせてもいいように感じます。それでも構わないならいいと思います。
申し訳ないのですが全然お話についていけていません。六種類コンバータができるのはなぜですか？　3種類×逆変換ですか？　変換の数だけ変換用のクラスが出来上がるのは不自然ではないように感じるのですが、これは悪い設計なのですか？

[Gurz1019_MP]

一つ認識の確認をさせてください。私は各DTOはすべて違うデータを表現したクラスだと思っているのですが、間違っていますでしょうか？

[Zuishin]

互換性のないオブジェクトということから、質問にあるのはすべて違うデータで間違いないと思います。しかし、よく聞いてみると、ストレージが違うだけの同じデータであり、違うデータにする意味はありません。違うデータ型を作るという設計自体が間違っています。
このデータの差異は Read と Write で吸収すべきもので、そこから object あるいは複数種類の TDto という違うデータを出力すべきではありません。

三種類のデータをコンバータに渡す場合、三種類それぞれから残りの二種類にコンバートする必要が出てきますので、コンバータは六種類になります。加えて Read 三種類、Write 三種類で合計十二のメソッドを実装しなければなりません。

Read で一種類のデータを出力し、Write でそのデータをそれぞれのストレージに保存するような設計にすれば、Read 三種類 Write 三種類の合計六種類で済みます。ストレージの数が増えれば増えるほどこの二つの設計の差は広がっていきます。

ストレージの種類は三種類と仮定していますが、DB を一種類と数えるのも問題があります。CSV と Excel が違うなら、SQL Server と MySQL も違うはずです。十種類のストレージを扱えるようにする場合、いったいどれだけのコンバータを実装しなければならないでしょうか？ また後で増えた場合、どれだけ面倒な作業になるでしょうか？ 良い設計とは思えません。

[Gurz1019_MP]

成程、各DTOが本質的には同じものを表現するクラスなら、Zuishin様のおっしゃるとおりだと思います。シリアライズに関しては、名前は聞いたことがあったのですがよく理解できていないところだったので大変勉強になりました。ありがとうございます。
ただストレージの対象がDBの時に、シリアライズを使った方法だとデータ構造が変わってしまわないか疑問です。シリアライズされたデータを格納するようにDBのデータ型が変わりませんか？　それともDBのデータ構造を維持したままシリアライズする（？）方法がありますか？

[Zuishin]

例えば CodeFirst を使ってもいいし、DataReader を使ってもいいし、方法は色々あると思います。

[Gurz1019_MP]

失礼、それはSerializable属性を使った方法とは違う方法ですか？

[Zuishin]

違います。Serializable 属性を使うだけがシリアライズではありません。

[Gurz1019_MP]

そうなのですね。では例えばEPPlusを使ってExcelデータを読み書きするクラスを作ったとすると、これもシリアライザと呼べるということですか？

[Zuishin]

データをシリアライズすることを目的とするならシリアライザでしょう。

本題は設計の話ですが、もしかして用語が間違っていると言いたいのですか？

[Zuishin]

あなたの回答に従った場合、ConcreteClass をデータの種類×(データの種類 - 1)だけ作らなければなりません。
何なんですかこれ？
継承する意味ありますか？
本当に質問の設計に比べて良い設計になっていますか？

10 種類のデータ型に対して具象型が 90 種類必要です。そこにデータ型を一つ増やすと具象型を 20 種類追加しなければなりません。

また、Dto および ConvertDto から ConcreteXXX を導き出す手段が無いので別にテーブルを作って初期化しなければなりません。

この二点だけでも私から見れば有り得ない設計なんですが、これを許容してでも採用できる利点はどこですか？

[Zuishin]

もう一つ。ポリモーフィズムが死んでいます。

AbstractClass はジェネリック型なので、この型に代入しようと思えば型引数が二つ必要になりますが、そこから派生する具象化クラスは一つしかありません。

例えば AbstractClass<DTO1, ConvertDto1> から派生するクラスは ConcrateClass1 のみです。

ですから AbstractClass<DTO1, ConvertDto1> に代入する意味がありません。ということは、例えば具象クラスのオブジェクトをループで扱うのにリフレクションが必要になるということです。

何のためのテンプレートでしょうか？

テンプレートの利点は一つのテンプレートから派生する無限の子孫をポリモーフィズムを用いて同一的手法で扱えることです。

しかしこの回答は逆に無限にあるテンプレートからたった一つだけの子孫を作るコードです。テンプレートの利点が生かせますか？

[Gurz1019_MP]

いえ、私自身がシリアライザという言葉に馴染みがなかったのでお聞きしたかっただけです。調べてもZuishin様がおっしゃっているものがシリアライザなのかどうかわからなかったので…　喧嘩を売っているように聞こえたなら申し訳ありません。
Zuishin様の説明で、この設計が悪い設計であることは理解できました。そもそも私はDTOを複数作らなければならないことが前提だと思っていたのでそこから間違っています。

Answer 2

データのシリアライズを柔軟に行うのはだいたいこんな感じでできます。

// 個別のデータ
public class Row
{
}

// データ全体
public class Data
{
    public string Title { get; set; }
    public DateTime LastWriteTime { get; set; }
    public IList<Row> Rows { get; } = new List<Row>();
}

// データを保持するクラス(これを作らず Data クラスと統合するのもあり)
public class Document
{
    public static void Convert(DataSerializer src, DataSerializer dst)
    {
        dst.Serialize(src.Deserialize());
    }

    public void Load(DataSerializer serializer)
    {
        Data = serializer.Deserialize();
    }

    public void Save(DataSerializer serializer)
    {
        serializer.Serialize(Data);
    }

    public Data Data { get; set; }
}

// 何も実装を含まないのでこれだけならインターフェースの方がいい
public abstract class DataSerializer
{
    public abstract void Serialize(Data data);
    public abstract Data Deserialize();
}

public class ExcelSerializer : DataSerializer
{
    public ExcelSerializer(string fileName)
    {
        throw new NotImplementedException();
    }

    public override void Serialize(Data data)
    {
        throw new NotImplementedException();
    }

    public override Data Deserialize()
    {
        throw new NotImplementedException();
    }
}