質問: new StreamWriter(path, true); } catch { throw new ConstructorException(FileOpenErrorLog.Instance); } } public void SendData(IEnumerable data) { try { // データをテキストファイルに追加 } catch { throw new SendDataException(FileAppendErrorLog.Instance); } } public void Dispose() { this._writer?.Dispose(); } } #endregion ``` 質問は以下になります。 1. MapDataで発生した例外は予期しないエラーとするべきか意見をください。 よろしくお願いします。
回答: こんにちは。 なんとなく思ったことを回答してみます。 solid原則とかのガイドラインはきちんと学習しているわけではないので、的外れな意見かもしれません。 まず、インターフェースは「書かれた以上の仕事をしない」のがルールです。 例えば、`IGetData`インターフェースですが、こんな感じの定義だと仮定してみます。 ``` interface IGetData { T GetData(); } ``` これであれば、「`GetData`メソッドは`T`型のデータを返す」という一文以上のことはしません。「データを返せない場合」があれば、それは「想定外の動作」となり、例外を送出する以外の選択肢は無いです。実装側の設計によっては、「データが無い場合は`default(T)`」というのも考えられますが、あまりいい設計とは思いませんね。 そして、`IGetData`インターフェースが最初から「データを取得できない可能性もある」パターンをサポートする場合、それをインターフェース定義に盛り込みます。例えば以下のような感じでしょうか。 ``` interface IGetData { // C#7のValueTuple風記述です。Tupleクラスを使ったり、Resultクラスとかを定義しても構いません。 (bool Success, T Data) GetData(); } ``` Successプロパティで成功の有無を判定する機能が盛り込まれています。これであれば、「取得失敗」は「例外的状況」ではなくなり、`GetData`の動作パターンの一つとなるわけです。 実際、`IGetData`をどちらの定義にするかは人によります。`GetData`メソッドはデータを返すのが「当たり前」であるなら、返せないのは「例外である」としてしまっても何の問題も無いのです。その時は、`GetData`メソッドを使用する`Do`メソッドの方で、「失敗した場合の例外処理」を記述しなければならないことになります。 個人的な感覚だと、`GetData`メソッドは引数を持っていないので、「データを取得できるのが当たり前」であると認識できるため、データを取得できないのは「例外的状況である」としてしまいます。 そして、`ParseData`メソッドはパースするデータを引数で入れているため、「引数によって成功するかどうかが決まる」となるため、戻り値に「成功したかどうか」の情報を盛り込みます。 次に、「異常時には終了」についてです。 まず、`Do`メソッドが「アプリケーション全体の終了を管理できる」というのは権限を持ちすぎです。 `Do`がやるのは「DataをGetしてParseしてSendする」だけであるべきで、取得できなかった場合の処理は`Do`メソッドが扱える範囲でしか記述してはいけません。アプリケーションの終了までを行いたいのであれば、各タスクの失敗を`Do`メソッドが認識した時点で例外を投げるしかないです。`Do`が「失敗したらアプリケーションを終了させたい」のであれば、それは`Do`の身に余る行為なので、こういう場合は予め定義しておいた独自例外をスローし、上位レイヤ(場合によってはエントリポイントまで遡る)でその例外をcatchし処理する方針になるはずです。このときの`Do`メソッドは、「ログを出力してから例外をスロー」よりは、エラーログを持たせる作りの独自例外を用意し、catchした場所で異常終了の事由をログに出力する形でも良いと思います。このエラーログは「アプリケーションがしぬほどの事由」に相当するので、それ相応のレイヤで扱うほうがスマートに見えるのです。 追記:01/20 想像でコード例を書いてみました。 IFixedLogがログの実体だと仮定しています。 部分的に適当なので、頑張って読み替えてください。 ``` static int Main() // エントリポイント { try { test.Do(); return 0; // 正常終了 } catch (FatalErrorException exception) // 致命的なエラーによる強制終了の場合は { var log = exception.Log; // 例外からLogを取り出す logger.Write(log); // ログを出力 return -1; } catch (Exception exception) // 予期しない例外の場合は { var log = CreateLog("予期しないエラー" + exception); logger.Write(log); return -1; } } // 独自例外 class FatalErrorException : Exception { public IFixedLog Log { get; } // 例外にLogを持たせる public FatalErrorException(IFixedLog log) : base() { this.Log = log; } } // 仮にこんなインターフェースで interface IGetData { // 必ずGetできる。失敗するのは例外的状況とする。 T GetData(); } interface IParseData { // 与えるrawによっては失敗する。成功した場合はDataの中に結果が入る。 (bool Success, TParsed Data) ParseData(TRaw raw); } interface ISendData { // 失敗しない。 void Send(T data); } class TestClass { public Test(IGetData getter, IParseData parser, ISendData sender) { // ... } public void Do() { var raw = default(TData); try { raw = _getter.GetData(); // GetDataは基本成功 } catch (GetDataException) // GetDataがGet失敗した時に出す例外「だけ」をcatch { var log = CreateLog("get失敗"); throw new FatalErrorException(log); // 致命的なエラーを例外で表現する } var parsed = _parser.ParseData(raw); // ParseDataは例外を出さない if (!parsed.Success) // parseが失敗した場合は { var log = CreateLog("parse失敗"); throw new FatalErrorException(log); // ログ入りスロー } _sender.Send(parsed.Data); } } ``` Doメソッドが自分のやるべきことだけをこなして、それ以外は上位に投げればいいというコードになっているところに注目して下さい。 追記:01/21 自アプリケーションが管理する例外は必ずログを持つという構造にしてみました。 ``` // アプリケーション例外のベースとなる抽象例外クラス public abstract class MyNantokaApplicationException : Exception { public IFixedLog Log { get; } protected MyNantokaApplicationException(IFixedLog log) : base() { this.Log = log; } protected MyNantokaApplicationException(IFixedLog log, string message) : base(message) { this.Log = log; } protected MyNantokaApplicationException(IFixedLog log, string message, Exception inner) : base(message, inner) { this.Log = log; } } // 例えば、アプリケーションを強制終了するほどの致命的なエラーを表す public class FatalErrorException : MyNantokaApplicationException { public FatalErrorException(IFixedLog log) : base(log) { } public FatalErrorException(IFixedLog log, string message) : base(log, message) { } public FatalErrorException(IFixedLog log, string message, Exception inner) : base(log, message, inner) { } } // GetDataがGetに失敗したときに出す例外とする public class GetDataException : MyNantokaApplicationException { public GetDataException(IFixedLog log) : base(log) { } } // Doメソッドの前半だけ public void Do() { var raw = default(TData); try { raw = _getter.GetData(); } catch (GetDataException exception) // GetDataがGetに失敗したということしか分からない { var log = exception.Log; // GetDataExceptionから実際に失敗した理由のLogを取り出す throw new FatalErrorException(log); // Get失敗は致命的なエラー扱いとして、そのログを入れてスロー } // ... } // ファイルを開くIGetDataのGetDataメソッドの例 public TData GetData() { try { /* ファイルオープン */ return data; } catch { var log = FileOpenErrorLog.Instance; // IFixedLogオブジェクトがこんな感じのシングルトンだと仮定 throw GetDataException(log); // ファイルオープン失敗のIFixedLog入りでGetDataExceptionをスロー } } // メモリから読み込むIGetDataのGetDataメソッドの例 public TData GetData() { try { /* メモリ読み込み */ return data; } catch { var log = MemoryReadErrorLog.Instance; throw GetDataException(log); // 読み込み失敗のIFixedLog入りでGetDataExceptionをスロー } } ``` この方法であれば、Doが「Getが失敗したこと」までしか把握していない状況で「ファイルオープン失敗」のログを正しく吐かせることができます。 追記:01/24 MapData回りの再設計例です。 ``` // IParseDataのリネーム interface IConverter { Tuple Convert(T data); } interface IValidateData { bool ValidateData(T data); } class CollectionParser : IConverter, IEnumerable> { private readonly IConverter _mapper; // 変数名はわかりやすさのため無変更で public CollectionParser(IConverter mapper) { this._mapper = mapper; } public Tuple> Convert(IEnumerable rawItems) { var mapped = rawItems .Select(_mapper.Convert) .Where(result => result.Item1) // 失敗をフィルタし、 .Select(result => result.Item2) // Dataを取り出し、 .ToArray(); // 配列化 return Tuple.Create(mapped.Length == rawItems.Count(), mapped.AsEnumerable()); } } // CollectionParserもSimpleMapperもどっちもIConverter。それぞれ意味は違うのでこうやって二重に使える。 class XXXSimpleMapper : IConverter { private readonly IValidateData _validator; // validatorはMapperの中に持たせる public XXXSimpleMapper(IValidateData validator) { this._validator = validator; } public Tuple Convert(string raw) { if (!this._validator.ValidateData(raw)) return Tuple.Create(false, default(Something)); // Validateが成功した以上、変換は必ず成功することを期待する。 // 検証したい項目を増やすなら、Validatorを増やせばいい。 var name = raw.Substring(0, 10).TrimStart(); var message = raw.Substring(10); return Tuple.Create(true, new Something(name, message)); } } // Validatorは特に変更してない class XXXXLogDataValidatorWithLog : IValidateData { private readonly IFixedLogger _logger; public XXXXLogDataValidatorWithLog(IFixedLogger logger) { this._logger = logger; } // ValidateDataは「Convert可能なデータである」ことを検証する。 // 例えば、Substringを使うConverterを想定するなら、Lengthのチェックも必要。別のValidatorを新たに実装しても良い。 public bool ValidateData(string data) { var result = data == string.Empty; // 失敗したらログを出力 if (!result) { _logger.Log(UnavailableDataDetectedErrorLog.GetInstance(data, "文字列が空文字")); } return result; } } ``` かなりシンプルかつ明瞭になったと思うんですが、どうでしょう? 追記:01/24_02 質問2のほうにも回答していきます。 まずは、質問内容以前の話なんですが。「ファイルは開始時にオープンして終了時にクローズすることになりました。」という設計がまずおかしいです。ファイルを常時オープンしっぱなしにする理由はなんでしょうか?外部リソースは「使用する時だけ開いて、すぐに閉じる」が安全に使用するための鉄則です。コードを見る限り、常時ファイルを開いておかなければならない理由は全く無いように見えます。終了時まで一切クローズしないのであれば、そもそもIDisposableの実装も冗長ということになります。質問のコードから読み取れない追加情報があったとしても、リソースの保持というのはかなり危ないコーディングなので、推奨しません。なんとかしてリソースの保持を行わないように可能な限り設計を見直します。 例えば、自分なら以下のように書きます。これ以外はないです。 ``` // データをファイルに送信するISendDataの実装 class PlainFileSender : ISendData> { private readonly string _path; public PlainFileSender(string path) { this._path = path; } public void SendData(IEnumerable data) { try { using (var writer = new StreamWriter(this._path, true)) { // データをテキストファイルに追加 } } catch { var log = FileAppendErrorLog.Instance; throw new SendDataException(FileAppendErrorLog.Instance); } } } ``` 一応、質問のほう、コンストラクタで発生させる例外についても書きますね。 エントリポイントを丸ごとtryで囲んでいるんですよね?であれば、インスタンスの生成時に「アプリケーションを継続できない状態」が発生するなら、コンストラクタ内でログを作成してFatalErrorをスローしてやればいいです。例外に「処理の流れ」なんていうものはなくて、その例外的状況「アプリケーションを継続できない」に相当するほどの異常事態が発生した際に、自分の作業をその場でぶん投げるためにあるのです。例外を書くときに「処理フロー」を考え始めるとドツボにはまるので注意して下さい。「やってみてなんかダメそうだったからもう投げる」「自分はこの例外が来るパターンは知ってるから、catchする体制だけ整えてある」という、直接相手のことを意識しないコーディングを心がけると良いです。