編集履歴

回答編集履歴

質問2への回答を追記

2017/01/24 09:59

投稿

スコア4110

test CHANGED Viewed

@@ -78,6 +78,8 @@
+追記:01/20
 想像でコード例を書いてみました。
 IFixedLogがログの実体だと仮定しています。
@@ -262,6 +264,8 @@
+追記:01/21
 自アプリケーションが管理する例外は必ずログを持つという構造にしてみました。
 ```csharp
@@ -629,3 +633,87 @@
 かなりシンプルかつ明瞭になったと思うんですが、どうでしょう？
+---
+追記:01/24_02
+質問2のほうにも回答していきます。
+まずは、質問内容以前の話なんですが。「ファイルは開始時にオープンして終了時にクローズすることになりました。」という設計がまずおかしいです。ファイルを常時オープンしっぱなしにする理由はなんでしょうか？外部リソースは「使用する時だけ開いて、すぐに閉じる」が安全に使用するための鉄則です。コードを見る限り、常時ファイルを開いておかなければならない理由は全く無いように見えます。終了時まで一切クローズしないのであれば、そもそもIDisposableの実装も冗長ということになります。質問のコードから読み取れない追加情報があったとしても、リソースの保持というのはかなり危ないコーディングなので、推奨しません。なんとかしてリソースの保持を行わないように可能な限り設計を見直します。
+例えば、自分なら以下のように書きます。これ以外はないです。
+```csharp
+// データをファイルに送信するISendDataの実装
+class PlainFileSender : ISendData<IEnumerable<Something>>
+{
+    private readonly string _path;
+    public PlainFileSender(string path)
+    {
+        this._path = path;
+    }
+    public void SendData(IEnumerable<Something> data)
+    {
+        try
+        {
+            using (var writer = new StreamWriter(this._path, true))
+            {
+                // データをテキストファイルに追加
+            }
+        }
+        catch
+        {
+            var log = FileAppendErrorLog.Instance;
+            throw new SendDataException(FileAppendErrorLog.Instance);
+        }
+    }
+}
+```
+一応、質問のほう、コンストラクタで発生させる例外についても書きますね。
+エントリポイントを丸ごとtryで囲んでいるんですよね？であれば、インスタンスの生成時に「アプリケーションを継続できない状態」が発生するなら、コンストラクタ内でログを作成してFatalErrorをスローしてやればいいです。例外に「処理の流れ」なんていうものはなくて、その例外的状況「アプリケーションを継続できない」に相当するほどの異常事態が発生した際に、自分の作業をその場でぶん投げるためにあるのです。例外を書くときに「処理フロー」を考え始めるとドツボにはまるので注意して下さい。「やってみてなんかダメそうだったからもう投げる」「自分はこの例外が来るパターンは知ってるから、catchする体制だけ整えてある」という、直接相手のことを意識しないコーディングを心がけると良いです。

MapData再設計例追記

2017/01/24 09:59

投稿

tamoto

スコア4110

test CHANGED Viewed

@@ -447,3 +447,185 @@
 この方法であれば、Doが「Getが失敗したこと」までしか把握していない状況で「ファイルオープン失敗」のログを正しく吐かせることができます。
+---
+追記:01/24
+MapData回りの再設計例です。
+```csharp
+// IParseDataのリネーム
+interface IConverter<in T, TResult>
+{
+    Tuple<bool, TResult> Convert(T data);
+}
+interface IValidateData<in T>
+{
+    bool ValidateData(T data);
+}
+class CollectionParser<TRawItem, TMappedItem> : IConverter<IEnumerable<TRawItem>, IEnumerable<TMappedItem>>
+{
+    private readonly IConverter<TRawItem, TMappedItem> _mapper; // 変数名はわかりやすさのため無変更で
+    public CollectionParser(IConverter<TRawItem, TMappedItem> mapper)
+    {
+        this._mapper = mapper;
+    }
+    public Tuple<bool, IEnumerable<TMappedItem>> Convert(IEnumerable<TRawItem> 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<string, Something>
+{
+    private readonly IValidateData<string> _validator; // validatorはMapperの中に持たせる
+    public XXXSimpleMapper(IValidateData<string> validator)
+    {
+        this._validator = validator;
+    }
+    public Tuple<bool, Something> 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<string>
+{
+    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;
+    }
+}
+```
+かなりシンプルかつ明瞭になったと思うんですが、どうでしょう？

例外の例

2017/01/24 00:59

投稿

tamoto

スコア4110

test CHANGED Viewed

@@ -255,3 +255,195 @@
 Doメソッドが自分のやるべきことだけをこなして、それ以外は上位に投げればいいというコードになっているところに注目して下さい。
+---
+自アプリケーションが管理する例外は必ずログを持つという構造にしてみました。
+```csharp
+// アプリケーション例外のベースとなる抽象例外クラス
+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が失敗したこと」までしか把握していない状況で「ファイルオープン失敗」のログを正しく吐かせることができます。

コード例を作成

2017/01/21 07:29

投稿

tamoto

スコア4110

test CHANGED Viewed

@@ -71,3 +71,187 @@
 まず、`Do`メソッドが「アプリケーション全体の終了を管理できる」というのは権限を持ちすぎです。
 `Do`がやるのは「DataをGetしてParseしてSendする」だけであるべきで、取得できなかった場合の処理は`Do`メソッドが扱える範囲でしか記述してはいけません。アプリケーションの終了までを行いたいのであれば、各タスクの失敗を`Do`メソッドが認識した時点で例外を投げるしかないです。`Do`が「失敗したらアプリケーションを終了させたい」のであれば、それは`Do`の身に余る行為なので、こういう場合は予め定義しておいた独自例外をスローし、上位レイヤ（場合によってはエントリポイントまで遡る）でその例外をcatchし処理する方針になるはずです。このときの`Do`メソッドは、「ログを出力してから例外をスロー」よりは、エラーログを持たせる作りの独自例外を用意し、catchした場所で異常終了の事由をログに出力する形でも良いと思います。このエラーログは「アプリケーションがしぬほどの事由」に相当するので、それ相応のレイヤで扱うほうがスマートに見えるのです。
+---
+想像でコード例を書いてみました。
+IFixedLogがログの実体だと仮定しています。
+部分的に適当なので、頑張って読み替えてください。
+```csharp
+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<T>
+{
+    // 必ずGetできる。失敗するのは例外的状況とする。
+    T GetData();
+}
+interface IParseData<TRaw, TParsed>
+{
+    // 与えるrawによっては失敗する。成功した場合はDataの中に結果が入る。
+    (bool Success, TParsed Data) ParseData(TRaw raw);
+}
+interface ISendData<T>
+{
+    // 失敗しない。
+    void Send(T data);
+}
+class TestClass<TData, TParsed>
+{
+    public Test(IGetData<TData> getter, IParseData<TData, TParsed> parser, ISendData<TParsed> 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メソッドが自分のやるべきことだけをこなして、それ以外は上位に投げればいいというコードになっているところに注目して下さい。