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

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

ただいまの
回答率

90.52%

  • C#

    7109questions

    C#はマルチパラダイムプログラミング言語の1つで、命令形・宣言型・関数型・ジェネリック型・コンポーネント指向・オブジェクティブ指向のプログラミング開発すべてに対応しています。

c#でsolid原則に基づく良いコードを書きたい

受付中

回答 4

投稿 編集

  • 評価
  • クリップ 5
  • VIEW 1,284

syogakusya

score 59

質問

以下が改善中の最新のコードです。

// データを取得
interface IGetData<out T>
{
    Tuple<bool, TParsed> ParseData(TRaw data);
}

// データを検証
interface IValidateData<in T>
{
    // 必ずGetできる。失敗するのは例外的状況
    T GetData();
}

// データを変換
interface IParseData<in TRaw, TParsed>
{
    // 与えるrawによって失敗。成功した場合Dataに結果が入る。
    bool ValidateData(T data);
}

// データをマッピング
// 必ずMapできる。失敗するのは例外的状況
// 失敗したら予期しないエラーにする?
interface IMapData<in TRaw, out TMapped>
{
    TMapped MapData(TRaw raw);
}

// データを送信
interface ISendData<in T>
{
    // 必ずSendできる。失敗するのは例外的状況
    void SendData(T data);
}

// アプリケーション例外のベースとなる抽象例外クラス
public abstract class MyNantokaApplicationException : Exception
{
    public IFixedLog Log { get; }

    protected MyNantokaApplicationException(IFixedLog log)
        : base()
    {
        this.Log = log;
    }

  //...
}

// GetDataがGetに失敗したときに出す例外とする
public class GetDataException : MyNantokaApplicationException
{
    //...
}

// SendDataがSendに失敗したときに出す例外とする
public class SendDataException : MyNantokaApplicationException
{
    //...
}

// MapDataがMapに失敗したときに出す例外とする
// 失敗したら予期しないエラーのほうがいい?
public class MapException : MyNantokaApplicationException
{
    //...
}

// GetParseSendFlowがデータの変換に失敗したときに出す例外とする
public class ParseFailedException : MyNantokaApplicationException
{
    //...
}

// オブジェクトの生成に失敗したときに出す例外とする
public class ConstructorException : MyNantokaApplicationException
{
    //...
}

class Program
{
    static int Main()
    {
        var logger = new FixedLoggerToFile();
        try
        {
            // 本当はリソースを保持したらだめ
            using (var sender = new PlainFileSender(@"C:\...")) // Send)
            {
                var getter = new FileDataGetter("..."); // Get

                var validator = new XXXXLogDataValidatorWithLog(logger);
                var mapper = new XXXSimpleMapper();
                var parser = new CollectionParser<string, Something>(validator, mapper, logger); // Parse

                var test = new GetParseSendFlow<IEnumerable<string>, IEnumerable<Something>>(getter, parser, sender); // 合体
                test.Do(); // 処理実行
            }

            return 0; // 正常終了
        }
        catch (MyNantokaApplicationException exception) // アプリケーション例外の場合は
        {
            var log = exception.Log; // 例外からLogを取り出す
            if (log != null)
                logger.Log(log); // ログを出力
            return -1;
        }
        catch // 予期しない例外の場合は
        {
            var log = UnexpectedErrorLog.Instance;
            logger.Log(log);
            return -1;
        }
    }
}

class GetParseSendFlow<TData, TParsed>
{
  //...

    public GetParseSendFlow(IGetData<TData> getter, IParseData<TData, TParsed> parser, ISendData<TParsed> sender)
    {
        //...
    }

    public void Do()
    {
        var raw = _getter.GetData(); // GetDataExceptionが起きても何もできないので素通し

        var parseResult = _parser.ParseData(raw); // ParseDataは例外を出さない

        if (!parseResult.Item1)// parseが失敗した場合は
        {
            //var log = ParseFailedAndInterruptErrorLog.Instance;  // お手上げなのでぶん投げる
            throw new ParsedFailedException(null); // 要件を満たすためにとりあえずnull
        }

        _sender.SendData(parseResult.Item2); // SendDataExceptionが起きても何もできないので素通し
    }
}

// ファイルから文字列のコレクションを取得するIGetDataの実装
public class FileDataGetter : IGetData<IEnumerable<string>>
{
    //...

    public FileDataGetter(string path /** , string ... **/)
    {
        _path = path;
    }

    public IEnumerable<string> GetData()
    {
        StreamReader reader = null;
        NetworkConnection networkConnection = null;

        try
        {
            try
            {
                // ネットワークに接続
            }
            catch
            {
                throw new GetDataException(ConnectToNetworkForFileErrorLog.Instance); // GetDataExceptionをスロー
            }

            try
            {
                // ファイルオープン
                reader = new StreamReader(_path);
            }
            catch
            {
                throw new GetDataException(FileOpenErrorLog.Instance); // GetDataExceptionをスロー
            }

            var list = new List<string>();
            //...
            return list;
        }
        finally
        {
            reader?.Dispose();
            networkConnection?.Dispose();
        }
    }
}

// コレクションのデータを変換するIParseDataの実装
class CollectionParser<TRawItem, TMappedItem> : IParseData<IEnumerable<TRawItem>, IEnumerable<TMappedItem>>
{
    //...

    public CollectionParser(
        IValidateData<TRawItem> validator,
        IMapData<TRawItem, TMappedItem> mapper,
        IFixedLogger logger)
    {
        this._validator = validator;
        this._mapper = mapper;
        this._logger = logger;
    }

    public Tuple<bool, IEnumerable<TMappedItem>> ParseData(IEnumerable<TRawItem> rawItems)
    {
        // 変換の成否に関する戻り値設定に使う
        var inputItemsCount = 0;

        // 検証と変換に成功したもののみで絞込
        var parseResult = rawItems.Select(
            rawItem =>
                {
                    // 生データコレクションカウント
                    inputItemsCount++;

                    if (_validator.ValidateData(rawItem)) // 検証してみる
                    {
                        // 検証OKならマッピングしてみる
                        try
                        {
                            var mapped = this._mapper.MapData(rawItem);
                            return new { Data = mapped, Result = true };
                        }
                        catch (MapException ex)
                        {
                            _logger.Log(ex.Log); // 失敗したらエラーログ
                        }
                    }
                    // 検証失敗やマッピングで例外の場合は失敗を返す
                    return new { Data = default(TMappedItem), Result = false };
                }).Where(mappingResult => mappingResult.Result).Select(mappingResult => mappingResult.Data).ToList();

        // 得られたコレクションの長さで判定
        return new Tuple<bool, IEnumerable<TMappedItem>>(parseResult.Count == inputItemsCount, parseResult);
    }
}

// 特定のソフトが出力するログを検証し、エラーログも出力するIValidateの実装
class XXXXLogDataValidatorWithLog : IValidateData<string>
{
    private readonly IFixedLogger _logger;

    public XXXXLogDataValidatorWithLog(IFixedLogger logger)
    {
        this._logger = logger;
    }

    public bool ValidateData(string data)
    {
        var result = data == string.Empty;
     // 失敗したらログを出力
        if (!result)
        {
            _logger.Log(UnavailableDataDetectedErrorLog.GetInstance(data, "文字列が空文字"));
        }

        return result;
    }
}

// 特定のソフトが出力するログをオブジェクトに変換するIMapの実装
class XXXSimpleMapper : IMapData<string, Something>
{
    public Something MapData(string raw)
    {
        string name;
        string message;

        try
        {
            name = raw.Substring(0, 10).TrimStart();
        }
        catch
        {
            // ここや
            throw new SimpleMapExeption("名前が変換できない", raw);
        }

        try
        {
            message = raw.Substring(10);
        }
        catch
        {
            // ここは、MapExceptionではなく、予期しないエラーになるべきか?
            // あるいは、ここでログを出力する?
            // その場合返すものがなくなる
            // IParseDataにかえて、ここでログをはきfalseを返す?
            // そもそもIMapいる?
            throw new SimpleMapExeption("メッセージが変換できない", raw);
        }

        return new Something(name, message);
    }
}

#endregion

#region ISendDataの実体
// データをファイルに送信するISendDataの実装 リソースを保持するのでよくない
class PlainFileSender : ISendData<IEnumerable<Something>>, IDisposable
{
    private readonly  StreamWriter _writer;

    public PlainFileSender(string path)
    {
        try
        {
            this._writer = new StreamWriter(path, true);
        }
        catch
        {
            throw new ConstructorException(FileOpenErrorLog.Instance); 
        }

    }

    public void SendData(IEnumerable<Something> data)
    {
        try
        {
            // データをテキストファイルに追加
        }
        catch
        {
            throw new SendDataException(FileAppendErrorLog.Instance); 
        }
    }

    public void Dispose()
    {
        this._writer?.Dispose();
    }
}

#endregion

質問は以下になります。

  1. MapDataで発生した例外は予期しないエラーとするべきか意見をください。
    よろしくお願いします。
  • 気になる質問をクリップする

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

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

    クリップを取り消します

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

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

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

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

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

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

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

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

    質問の評価を下げる

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

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

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

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

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

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

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

    詳細な説明はこちら

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

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

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

回答 4

+5

こんにちは。
なんとなく思ったことを回答してみます。
solid原則とかのガイドラインはきちんと学習しているわけではないので、的外れな意見かもしれません。

まず、インターフェースは「書かれた以上の仕事をしない」のがルールです。
例えば、IGetDataインターフェースですが、こんな感じの定義だと仮定してみます。

interface IGetData<T>
{
    T GetData();
}

これであれば、「GetDataメソッドはT型のデータを返す」という一文以上のことはしません。「データを返せない場合」があれば、それは「想定外の動作」となり、例外を送出する以外の選択肢は無いです。実装側の設計によっては、「データが無い場合はdefault(T)」というのも考えられますが、あまりいい設計とは思いませんね。
そして、IGetDataインターフェースが最初から「データを取得できない可能性もある」パターンをサポートする場合、それをインターフェース定義に盛り込みます。例えば以下のような感じでしょうか。

interface IGetData<T>
{
    // 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<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メソッドが自分のやるべきことだけをこなして、それ以外は上位に投げればいいというコードになっているところに注目して下さい。


追記: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<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());
    }
}

// CollectionParserSimpleMapperもどっちも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;
    }
}

かなりシンプルかつ明瞭になったと思うんですが、どうでしょう?


追記:01/24_02
質問2のほうにも回答していきます。

まずは、質問内容以前の話なんですが。「ファイルは開始時にオープンして終了時にクローズすることになりました。」という設計がまずおかしいです。ファイルを常時オープンしっぱなしにする理由はなんでしょうか?外部リソースは「使用する時だけ開いて、すぐに閉じる」が安全に使用するための鉄則です。コードを見る限り、常時ファイルを開いておかなければならない理由は全く無いように見えます。終了時まで一切クローズしないのであれば、そもそもIDisposableの実装も冗長ということになります。質問のコードから読み取れない追加情報があったとしても、リソースの保持というのはかなり危ないコーディングなので、推奨しません。なんとかしてリソースの保持を行わないように可能な限り設計を見直します。
例えば、自分なら以下のように書きます。これ以外はないです。

// データをファイルに送信する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する体制だけ整えてある」という、直接相手のことを意識しないコーディングを心がけると良いです。

投稿

編集

  • 回答の評価を上げる

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

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

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

  • 回答の評価を下げる

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

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

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

  • 2017/01/20 17:18

    異常終了する場合の実装については、戻り値ベースではなく例外ベースのほうがよいということでしょうか?
    エントリポイントMainからこのDoメソッドが呼ばれるものとして、雰囲気だけでも例をいただきたいです。
    前述の厳密に決められたログの出力については、IFixedLogオブジェクトを受け取ってフォーマットしてログ出力するIFixedLoggerインターフェースを定義して実装しています。
    例外ベースにした場合、catchでこのロガーにオブジェクトを渡せればよいのですが、各IFixedLogの実装クラスがSimpleFixedLogBaseから派生しているため、例外クラスを継承できません。なので今は下位のクラスでエラーを検出したらその場でログを出力し、戻り値ベースでエラーとしてさかのぼっています。その場でログを出力したあと、例外をスロー(catch内での出力なら再スロー)することも考えたのですが、エントリポイントのcatchにひっかかることになり、予期しないエラーとしてもう一度ログが出力されてしまいます。

    キャンセル

  • 2017/01/20 17:42

    そうですね、メソッドの役割を超えて「異常時に動作を停止させる」を目的とするなら、その処理は例外で行うべきでしょう。
    幾つか分からない点を質問させてください。
    IFixedLogオブジェクトというのは、「プログラム内で扱うログの実体そのもの」であってますか?そして、そのIFixedLogオブジェクトを「実際にログとして出力」するクラスがIFixedLogger、ということでいいですか?

    キャンセル

  • 2017/01/20 18:32

    コード例を作ってみました。

    キャンセル

  • 2017/01/20 19:11

    ありがとうございます。
    帰宅次第いただきましたコードとご質問を含めて返答させていただきます。

    キャンセル

  • 2017/01/21 11:13

    ご質問に関してですが、その通りです。仕様書に定義されているログのパターンの数だけIFixedLog実装オブジェクトが存在し、IFixedLogオブジェクトを受け取ってログ出力を行うのがIFixedLoggerインターフェースの役割です。
    いただきましたソースコードを確認させていただきました。
    ありがとうございました。大変参考になりました。
    異常終了に関する制御については、戻り値をベースに制御するコード自体は作成したのですが、やはり無理があることを実感したため(Doメソッドにおいて、取得したデータがnullだったら異常終了扱いでfalseを返すというコードを書いていたとき、何かがおかしいことを悟りました)、いただいたサンプルの構造を参考にしながら、例外をベースに実現する方針でリファクタリングすることを考えています。
    このセオリーで問題を解決しようと考えた場合に、いくつか意見をお聞かせいただきたい点がございます。
    まず、ログに出力する内容は仕様書により一言一句まで定義されており、機能ではなく実装よりの内容だということです。
    つまり、ファイル取得に失敗した場合には"ファイルのオープンに失敗しました。"のように出力しなければなりません。(実際にはログのIDも出力しなければならないため、各IFixedLogはシングルトンのイメージです)
    いまの構造ですとDoメソッドがログの出力内容を決めていますが、IGetDataインターフェースはGetDataExceptionにのみ依存しているため、Doメソッドでの捕捉可能なIGetDataからの例外はGetDataExceptionのみかと思います。
    GetDataExceptionを継承するFileOpenExceptionを定義し、そこにログの内容を持たせることを考えたのですが、それでは結局GetData実装クラスがログに対する厳密な出力を意識することになってしまいます。
    このような場合、どのようにコードを変更するのがよいでしょうか?
    こういった特殊な場合は普通の手段では対応できないといった旨の回答でも構いませんので、ご意見を頂ければと思います。
    よろしくお願いします。

    キャンセル

  • 2017/01/21 13:12 編集

    うーん?何に悩んでるのかがちょっと分からないんですが……。
    まず、"オープン失敗Log"というのは、「GetDataがGetできなかったときのログ」という定義になっているのですか?それであれば、Logオブジェクトの取得はGetData内で行うべきです。Logオブジェクト入りの例外を作成し、それをスローすればよいでしょう。逆に"オープン失敗Log"の定義が「Do内でGetDataが失敗したときのログ」となっているなら、その定義の通り、Logオブジェクトの取得はDoメソッド内で行いましょう。
    しかし、そのどちらであっても「Getできなかったときにアプリケーションを終了させたい」という要件を持っているのは、Get命令を投げているDoメソッドのほうだと思います。GetDataには「DataをGetする、Getできなかったら例外」という定義しかないです。GetDataの例外はあくまで「Getできなかった」という異常事態を報告するだけにした方が設計としてはスマートなのです。
    次に、回答コード中のGetDataExceptionというのは、「Getに失敗した」という例外をDoが補足するために、GetDataが出す専用の独自例外を定義しただけのものです。まず、例外機構には「そのメソッドが想定する例外以外はcatchしてはいけない」という大前提があります。たとえばDoメソッド内にcatch (Exception) { ... } と書いてしまうと、Doが「GetDataがどんな例外で失敗することも想定している」となってしまいます。Doが想定しているのはあくまで「GetDataが『Getに失敗』したかどうか」だけなので、これ以外の例外もcatchしてしまうと、意図せず「例外の握りつぶし」が発生してしまう可能性があります。ちゃんとGet失敗のみを認識できるのであれば、GetDataExceptionなんてものを定義しなくても、たとえば既存のIOExceptionなどを使う仕様にしても大丈夫です。catch (Exception) を使っていいのは、本当に「どんな例外が出ても同じように扱いたい」という場合だけです。それか、「例外が発生==失敗である」と決めつけたような例外フローを構築する場合ですね。これは汎用フレームワーク内ではよくやりますが、ユーザーコード中ではあんまりこれが必要な状況は少ないのではないでしょうか。
    コメントの話を回答のコードに入れ込むなら、単純にDoメソッド内catchの中で「CreateLogでGet失敗ログオブジェクトの作成」としているところを、「IFixedLogの、オープン失敗ログを表すオブジェクトのシングルトンインスタンス取得」とするだけではないでしょうか?何か根本的な勘違いがあったらすみません。質問があれば遠慮なくどうぞ。

    キャンセル

  • 2017/01/21 14:58

    プログラム設計はなく、処理フロー以外は何も決まっていません。
    業務の形式として、処理フロー(と出力されるログや動作を定義する資料)を渡されたらそれを満たすソフトを作るだけです。(手順としては普通逆なのかもしれませんが。今の部署しか知らないのでわかりません)
    "ファイルのオープンに失敗しました。"というログは、ファイルのオープンに失敗したときに出力されるログとして資料に定義されていて、それを実現するためのプログラム設計は私が決めます。そして、その設計で悩んでいるというのが今回の質問となります。
    「GetDataがGetできなかったときのログ」とするのも「Do内でGetDataが失敗したときのログ」とするのも自由なのですが、いただいたご意見などから「Do内でGetDataが失敗したときのログ」としたいと考えています。しかし、Doメソッドが知っているのは"IGetDataが何らかの方法でデータを取得すること"だけです。そうすると、Doメソッドで「Get失敗」といった旨のログを出すことはできても、「ファイルのオープンに失敗」というログを出すことはできないのではないかと思い、先の質問をさせていただきました。一応、私のほうで懸念のあるコードを載せておきます。

    キャンセル

  • 2017/01/21 16:09 編集

    なるほど、なにを気にしているのか分かりました。Doメソッドが知ることができるのは「Getが失敗したということ」までですが、投げたいログは「ファイルのオープン失敗」である、そして、IGetDataの実体は「ファイルからGet」かもしれないし、「メモリからGet」かもしれない。Doメソッドはそれを知ることができないでしょう?という話ですね。
    では、こういう設計はどうでしょう?
    「アプリケーションのコード内で、rethrow以外で自分がthrowする例外は、全て、ログを保持する抽象例外クラス『MyNantokaApplicationException』を継承した型のみとする」
    例を回答に追記してみます。->追記しました。

    キャンセル

  • 2017/01/21 18:07 編集

    はい、その通りです。
    コードを確認いたしました。本当にありがとうございます。視界が開けた思いです。
    この例外の使い方をしっかりと身に着けようと思います。
    ご提案いただいた方法を設計の方針としたいのですが、もう何点か、主に設計方針のことで質問させていただきたいことがあります。
    後ほどコメントと質問内のコードについて少し編集をいたしますので、よろしければまたご意見お聞かせください。
    よろしくお願いいたします。

    申し訳ありませんが、質問本文を触る前に、一点質問させてください。
    このような設計になった場合、IParseDataのParseDataに失敗したときについてはどのようにログを出力すればよいのでしょうか?

    キャンセル

  • 2017/01/22 01:48 編集

    ふむ……もしかしてIParseDataもparserの実体の種類によって出すログを変えたい要件があったりしますかね?parserの種類が関係しないログであれば、回答のコードのように「if文でParseに失敗していたときに"parse失敗"ログを取得し例外をスロー」だけで済むんですが……。
    Parseは成功も失敗も普通にあり得る処理で、失敗が「想定の範囲内」となるため、「想定外」を通知するためにある例外フローは使うべきではない処理ということになります。なにも「例外を使ってログを伝搬できるようにした」からといって、「ログを取り扱うときには例外を使わなければいけない」というわけではないのですよ。
    では、実際にParseDataが「中身」に依存するログを吐きたいという要件があるものと仮定してみます。ロギング方法ですが、ParseDataメソッドの戻り値のシグネチャを以下のようにするのはどうでしょう?

    (bool Success, TData Data, IFixedLog Log) ParseData(TRaw raw);

    ちょっと手抜きで、使用者の良心に依存する仕様ではありますが……「Successがtrueの時はDataにParseされたオブジェクトが入る」「Successがfalseの時はDataは空で、LogにIFixedLogオブジェクトが入る」という、Nullable<T>っぽい設計をちょっと拡大したやり方です。このやり方であれば、ParseDataの使用者がParserの中身に言及することなく、正しいログを出力できるようになります。
    継承と抽象メソッドを使って、もう少し安全に処理を記述できるようにするテクニックもありますが……コメント欄に書くのはちょっとつらいので、もし必要そうであればコード例を作成しますので言ってください。

    Parseの話からはちょっと逸れますが、「失敗したときにログを出力するだけして処理を継続」という要求仕様は普通にあり得ると思いますが、これを実現するとなると、「メソッド自体がloggerへの参照を持ち、ログへの出力処理を入れ込む」とかの泥臭いやり方が現実的になってしまいます。こういう要件が入ってくると、コードを完全にクリーンに保つのは大変難しいんですよね……。

    キャンセル

  • 2017/01/22 10:12

    >もしかしてIParseDataもparserの実体の種類によって出すログを変えたい要件があったりしますかね?
    実は今のところそういった要件はないのですが、少しでも多くのことについて学び取りたいと思い、お聞きしました。
    現在のデータ変換時の処理フローは以下になっています。
    ・ファイルから取得したデータ(行のコレクション)をデータべースに登録可能な形式(データのコレクション)に変換すること。このとき、変換できない行があった場合、その一行一行につき『変換不可能な行が見つかりました。 行内容: ~ 変換不可能な部分: ~』といったログを出力し、アプリケーションを異常終了扱いで終了すること。

    >parserの種類が関係しないログであれば、回答のコードのように「if文でParseに失敗していたときに"parse失敗"ログを取得し例外をスロー」だけで済むんですが……。
    実際にはparserが失敗したときに出力されるログは上記のものだけです。
    100行分失敗したら上記のログが100回出て、アプリケーションが終了することになっています。
    このログはどちらかというと異常終了することが確定しているログというわけではなく、このログを出したうえで変換に成功したログについてはデータベースに保存するといったフローに変更される可能性があります。
    ただし、ログの出力自体はどちらも同じレベルで、出力先やログの種類などに関する違いはありません。(どちらもErrorという種類です)
    現在このログはParseDataの中で利用しているbool IValidate<string>の実体であるValidateWithLogクラスのインスタンスで出力しています。ロガーはコンストラクタでインターフェースを渡し、ログの実体は例のシングルトンを直接参照しています。(可変部はログの実体のプロパティに設定しています。)
    そのため、現在の私のコードではParseDataで失敗した場合にはDoメソッドでそのままfalseを返して終了しています。例外を投げてしまうと、エントリポイントでcatchしてしまうためです。(現在の私のコードではDoメソッドの戻り値はbool値となっています。また、エントリポイントで全体をtry-catchし、捕捉した場合は予期しないエラーとして処理する機構は私のコードでも元からありました)

    >実際にParseDataが「中身」に依存するログを吐きたいという要件があるものと仮定してみます。ロギング方法ですが、ParseDataメソッドの戻り値のシグネチャを以下のようにするのはどうでしょう?
    (bool Success, TData Data, IFixedLog Log) ParseData(TRaw raw);

    前述のデータ変換時のエラーログ出力の実装と絡んだ質問なのですが、エラーログは必ず上位の層で出力されなければならないでしょうか?
    bool IValidate<string>の実装でそのエラーログを出力しても問題ないのであれば、Doメソッドやその上位の層が出力するログは存在しないのですが、その場合はどのようにエラーログの出力はないがアプリケーションを異常終了するという動作を実現すればよいかということが問題になるかと思います。

    >「失敗したときにログを出力するだけして処理を継続」という要求仕様は普通にあり得ると思いますが、これを実現するとなると、「メソッド自体がloggerへの参照を持ち、ログへの出力処理を入れ込む」とかの泥臭いやり方が現実的になってしまいます。
    ご想定された内容とは少し異なりますが、前述のとおり、「失敗したときに失敗の詳細をログに出力する」という要求を満たすために、すでにValidatorWithLogクラスではコンストラクタでIFixedLoggerを受け取ってしまっています。
    このログは、「アプリケーションがしなない程度の事由」に相当すると考えていて、『変換に成功したものだけはデータベースに保存する」といったふうにフローを変更されたとしても、このログは変更されないものと思います。
    これもまた前述のデータ変換時のエラーログ出力と関係する質問になりますが、「アプリケーションがしなない程度の事由」に相当するエラーログに関しては、どのように扱われれるべきなのでしょうか?

    キャンセル

  • 2017/01/22 14:36 編集

    なるほど、アプリケーションに影響するレベルではない、各処理単位でのログの出力は、その処理の中で行ってしまっていいと思います。ただし、ロガーのインスタンスは外から与えて差し替えができる設計になっていた方がスマートかなと思う程度です。parserが行単位でログを出力するというなら、ParseDataの戻り値は(bool Success, TData Data)で十分ですね。呼び出し元には「成功したかどうか」が伝われば十分です。

    Doの戻り値について。
    「ParseDataが失敗」は「致命的なエラー」として扱う(アプリケーションを終了させる理由になりえる)のですよね?そうであれば、DoメソッドのParseセクションでは、失敗したときその理由のログを作成し、例外を投げるべきだと思います。戻り値がboolであり、それをfalseとするのは「ParseDataが失敗するというのはDoメソッドの想定内の動作」ということになってしまいます。もしParseが失敗したときDoメソッドがfalseを返すとしたら、それは、Doメソッドの失敗時のプログラムの取り扱い責任がDoメソッドを呼び出す側に移動してしまうということになります。それはもはや「ParseDataの失敗」とは何の関係もないレイヤです。Doの戻り値をbool型にする場合は、そのboolには「"Doメソッドが" 成功/失敗した」以上の意味を持たせてはいけないのです。「メソッドの失敗があり得て、失敗した場合にその後の処理をどうするかを判断するのは呼び出し元の仕事」であるときだけ、戻り値をboolにするものです。

    例外というのは「catchされる前提でスローするものではない」という認識が重要です。例外というのは文字通り「例外」なので、スローするときは「自分には手に負えない、無理、アプリケーションが死んだら自分のせい」ということを表明するものです。上位がそれをリカバリーできる場合もあるし、できない場合もある。例外とはそういう仕組みなのです。

    では、実際の提案ですが、ParseDataメソッドは、parse失敗時は各行詳細ログをメソッド内で出力し、Successをfalseにして返します(例外は出さない)。そして、Doメソッドでは、Successがfalseであるとき、「Parseが失敗したからしにます」というログを新たに作り、それを元にして「致命的なエラー」として例外をスローします。(上のコード例であれば、FatalErrorExceptionで例外を出せば、予期しないエラーとして処理されることは無くなるはずです。)
    「ログそのものは例外とは関係なく出力される、しかし例外を出すときには必ずその理由のログが存在する」という関係を保てばよいと思います。

    キャンセル

  • 2017/01/22 15:16

    なるほど、わかりました。ではParseDataの失敗については、下の層でログを出力することにします。ロガーはインターフェースをコンストラクタで渡しているのでインスタンスの差し替えは可能です。
    はい、戻り値もそちらにしようと思います。

    ありがとうございます。考え方が分かってきた気がします。
    確かに、Doメソッドについて想定されているのは、一連の処理が全て正常に実行されることのみだと思います。そうであれば、その一連の処理について失敗したと判断したときには、例外を通じて上のレイヤに通知するのがDoメソッドの責任ということですね。
    「ログそのものは例外とは関係なく出力される、しかし例外を出すときには必ずその理由のログが存在する」という関係についてなのですが、これは本当に偶然というか、かなり特殊なケースだろうということは承知しているのですが、ご意見を頂ければと思います。データの変換に失敗した場合には、失敗した各行について『変換不可能な行が見つかりました。 行内容: ~ 変換不可能な部分: ~』というログのみを出力し、異常終了はするもののそれ以上のログ出力は行わないものとなっています。つまり、ParseDataに失敗した場合にDoメソッドが「致命的なエラー」をスローする際のログが存在しないのです。こういった場合、どういった方法で解決すればよいでしょうか?個人的に思いつくのは、独自例外について、「インスタンス内のログがnullなら例外の出力は行わない」としたり、MyNantokaApplicationNoLogExceptionを定義してエントリポイントでキャッチしたり、あるいはExceptionのインターフェースをvoid LogBy(IFixedLogger logger)のようにしたりといった変更を行ったりといったことなのですが、よりよい方法はあるでしょうか?
    些末な質問ですみません。ここまでお付き合いいただき本当にありがとうございます。

    キャンセル

  • 2017/01/22 15:44

    うーん……あくまでParseData内のログは「変換不可能な行が見つかった」というだけなので、異常終了するようなものではないですよね。となると、Doメソッドが「Parseに失敗したことによりどうしようもなくなった」という、「異常終了するに値する理由」が存在しないといけないと思います。なので、「Parse失敗」を表す新たなログオブジェクトを定義するほうが統一感があります。

    「ログがない」とするのは、言い換えると「ログに残すようなことはなかったけど、異常終了しました。」という意味になるので、非常にまずい設計だと感じます。本当に本当に、プログラム要件上そうするしか方法がない、というわけでなければ、絶対に「終了理由」を作成するべきだと思います。

    その本当にどうしようもない状況なら、Logをnullにする方法などが現実的かなと思います。

    キャンセル

  • 2017/01/22 16:54 編集

    わかりました。ありがとうございます。
    仕様として、どうしようもないようであれば、その時はLogをnullにし、エントリポイントではLogがnullでなければ出力するように変更しようと思います。
    それでは、現状で私が考えているソースコードと残りの疑問についていったん質問本文に載せてみようと思います。

    とりあえずコードのみ載せました。
    文字数が10000文字しか入らず、日本語がおかしい部分があることをご了承ください。
    質問は別でまとめ、また質問本文を編集しようと思いますが、現時点で変なところがあれば(もし読んでいただけるのなら、ですが)、指摘していただきたいです。
    引き続きよろしくお願いいたします。

    キャンセル

  • 2017/01/23 11:43

    現状でかるーく目を通してみました。
    一通り見た感想では、とても良くできていると思いました。十分に構造化できており、責任の分割もはっきりしているため、変更する際も手を入れやすいコードになっていると感じました。

    せっかくなので現時点で少し気になった点を2点、挙げてしまいます。

    まず、ValidateとMapが分かれているのはやや冗長感があります。Validateが元データを隅々まで検証した時点で、ほぼMap処理も完了している状況になるのではないでしょうか。MapDataメソッドが引数TRawを取り、その内容によっては失敗があり得るメソッドなので、この場合なら、Validate処理をMapDataメソッドに統合して、戻り値をTuple<bool, IMapped>とする、例外を送出しないように設計、失敗をboolで返すようにしてしまえば良いと思います。そうすれば、大元のParseDataメソッドが以下のようにかなり単純化できるようになります。

    public Tuple<bool, IEnumerable<TMappedItem>> ParseData(IEnumerable<TRawItem> rawItems)
    {
    var mapped = rawItems
    .Select(_mapper.MapData)
    .Where(result => result.Item1) // 失敗をフィルタし、
    .Select(result => result.Item2) // Dataを取り出し、
    .ToArray(); // 配列化
    return Tuple.Create(mapped.Length == rawItems.Count(), mapped.AsEnumerable());
    }

    2つ目、コメントにあった、FileOpenExceptionの話ですが、GetDataExceptionを抽象例外として、継承して実体例外を作成するかどうかは、趣味でどちらでも構わないと思います。自分の主観を述べると、もしもFileOpenExceptionだけをcatchし、ファイルのIOだけを特別扱いするような場面が存在するなら、例外を細分化する意味がありますが、おそらくそれはないと思うので、自分だったらGetDataExceptionを実体例外に変更し、直接インスタンス化するかなーと思います。

    キャンセル

  • 2017/01/23 23:57

    ご指摘ありがとうございます。
    それはよかったです。コードが改善されたのは、多くの質問に答えていただいたおかげです。ありがとうございました。お付き合いいただける間、学ばせていただいたことは精一杯コードに反映させようと思います。

    一点目についてですが、ValidateをMapDataに統合し、Tuple<bool, IMapped> MapData (IRaw)とした場合、シグネチャがParseDataと一緒になってしまうのですが、IMapDataとIParseDataはどちらも必要でしょうか?

    また、実際にはValidateではデータの検証と異常なデータに関するログ出力、Mapperではデータについてエンコードしたりいくつかのデータを加工したりしてオブジェクトを生成といったことをしているのですが、大きさというか責務の分離というか、そういった原則の観点から一つのクラスすることに問題はないでしょうか?
    もともとある本に載っていたパターンを真似したのですが、どちらか一方のインスタンスを差し替えたくなるような状況というのが思いつかないのも確かです。(実際のそのコードでは、ParseDataのループ内で各stringをsplitし、MapとValidateではstring[]を受けとるようになっていたので、少し違いますが)
    逆に、ParseDataを検証と変換に分けたほうがいいようなパターンもあるのでしょうか?

    2点目について、承知いたしました。さしあたっては、インターフェースのメソッドに割り当てられている例外を直接インスタンス化してスローするようにします。

    キャンセル

  • 2017/01/24 09:21 編集

    あー、たしかに、そこらへん考えてみると、ValidateとMapを分けたいという意図がわかりました。「Validateした上でMapしたいデータ」というのを「シリアライズ相当に複雑なデータ構造」もイメージしていたため、そこら辺に認識のズレがあったようです。

    しばらく解決法を考えてみましたが、こういうのを思いつきました。

    * IMapDataを廃止して、現状のIMapDataの担当箇所にもIParseDataを用いる。
    * IValidataDataは、IMapData(IParseData)と同じレイヤで扱わず、IParseDataの「中」で使用する。
    * IParseDataという名前をもう一段階抽象化して、interface IConverter<in T, TResult> { Tuple<bool, TResult> Convert(T data); } のように、「(データを)変換し、成功、或いは失敗を返す」ことだけを表すインターフェースとする

    つまり、「XXXSimpleMapperはメンバにIValidateDataを持ち、MapDataメソッド(Convertメソッド)は成功をbool値で返す(例外は出さない)、という設計です。この方法であれば、IValidateDataは一つにつき一つのバリデーションのみを担当するように設計でき、シリアライズなどの複雑なConvert処理には「IValidateDataをメンバに複数持たせる」ことで解決を図ることができます。
    ちょっと説明が難しくなってきたのでコード例で示します。-> 追記しました。

    2つ目の質問についてはまた後で書きます。 -> 書きました。

    キャンセル

  • 2017/01/24 20:42 編集

    回答ありがとうございます。
    毎度サンプルまでいただき、本当に恐れ入ります。しっかりと読ませていただきました。

    1つ目のMapData周りの設計につきましては、まだ少し聞きたい点があります。
    コードのパスと例外に関する意図は汲み取ることができ、納得もできたので、インターフェースは変更せずに同じようなコードパスを実現し、その上で設計の良し悪しを判断するためのロジックの部分について考える機会をいただきたいです。後ほど質問本文のコードを変更した上で、改めて疑問点をまとめようと思います。

    2つ目のコンストラクタにおける例外について、二つに分けてコメントさせていただきます。
    まずはリソースに関するご指摘についてですが、ありがとうございます。非常に勉強になりました。これまではログの実装もファイルをオープンしたままで行っていましたが、これは原則危険という認識なのですね。今後の開発・設計における課題として頭に入れておこうと思います。
    しかし、この設計を変更することができるかと言われると、不可能です。これは業務上の都合なのですが、今回のプロジェクトでは私は設計には関わっておらず、資料として渡されている大量の処理フローは、全てお客様を通しているようです。比較的多いモジュールの全てに処理フローの資料が存在し、各処理フローに開始時でファイルをオープンし、終了時にクローズすることが明記されています。私ごときには何ともできません。せっかくサンプルまでいただいたのに、コードを改善できず申し訳ありません。次に私にソフトウェア設計の裁量権があるときには、必ず反映させようと思います。
    次にコンストラクタでの例外に関するご回答への返答になりますが、例えばFileDataGetterなどのコンストラクタでも、アプリケーションを継続できないと考えたらFatalErrorをスローしてしまってよいのでしょうか?GetDataではGetDataExceptionしか投げられませんが、コンストラクタでは致命的なエラーが起きているということを判断してもよいのでしょうか?私のめちゃくちゃな考えは、以下の通りです。
    『エントリポイントがレイヤ1、GetParseSendFlowがレイヤ2、下々のやつがレイヤ3で、レイヤ3のやつにはアプリケーションの自分の仕事をして、想定外なことが起きたらインターフェースによって定められた例外を投げる。レイヤ2のやつが処理を実行できないと判断したらFatalErrorを投げる。レイヤ1のやつはFatalError以外の失敗は予期していない。つまり、予期される例外の各々をどう扱うかは、レイヤ2の管轄である。下々のオブジェクトを利用して全体の処理を管理し、処理を続行不可能な予期される例外が起きたときにはFatalErrorを投げるのがレイヤ2のやつの仕事である。PlainFileSenderはレイヤ3のやつだから、FatlErrorのような全体に関わる例外を投げてはいけない。しかし、SendDataExceptionはインターフェースに紐づく例外なので、そんな例外をコンストラクタで投げるわけにいかない。だが、例外は予期されるものとされている(ログが定義されている)ため、例外はきちんと投げなければいけない。では、コンストラクタのExceptionであるConstructorException : MyNantokaApplicationExceptionを例外が予期されるコンストラクタから投げるようにして、エントリポイントからオブジェクト生成処理をActionとして受け取って実行し、そこで発生したConstructorExceptionをcatchしてFatalErrorを投げるレイヤ2のConstructionExecutorを定義にすれば辻褄が合わせられるのでは…』
    特に例外とインターフェースと実装(クラス)とか、例外周りの考え方がまだまだ貧弱で乏しい(あるいは一部欠けている)のだと思います。
    度々お世話をおかけいたしますが、何卒考え方を広げていくための機会をいただければと思います。
    こちらのほうについても、ご提案いただいきましたコンストラクタで普通にFatalErrorをスローするパターンを、確認のためにコードに反映させようと思います。
    宜しくお願い致します。

    追記:コンストラクタで失敗ではなく、コンストラクタがStreamReaderを受け取る場合などで、エントリポイント上で例外が予期される場合も、エントリポイント上でFatalErrorを投げればそれでよいのでしょうか?

    キャンセル

  • 2017/01/25 11:00

    しばらく考えてみましたが、ちょっと回答するときの考慮が足りていませんでした……。最初の例外によるアプリケーションの管理の説明の際、簡潔にするため「FatalErrorException」という名前を付けたのが良くなかった気がします。例外をスローするときは「catchされるときの事を考えてはいけない」というルールがあります。つまり、本当は「致命的な例外が発生した」ではなく、「例外が発生したが、それを誰も処理できず、結果致命的になった」とするべきだったのです。混乱させて大変申し訳ないです。例外の名前に載せてもよかったのは「何が原因で、そのタスクの継続が不可能になったか」であって、「この例外が発生した結果、何が起きるか」ではなかったのです……。
    書き換えるとしたら、以下のように整理することを提案します。

    * FatalErrorExceptionは名前が不適当なため削除します。
    * エントリポイントのFatalErrorExceptionのcatch部分は、アプリケーションの例外そのものであるMyNantokaApplicationExceptionのcatchに書き換え、「アプリケーションが出した例外の内、誰も処理できなかった結果エントリポイントまで到達してしまった場合」の処理として扱います。
    * DoメソッドはGetDataExceptionをFatalErrorExceptionで包み直す必要はなくなって、「自分では(GetDataExceptionは)処理しきれないものなので、try/catchせず素通し」としてしまいます。ParseDataのほうは、ParseFailedExceptionなどを定義しておき、それをスローします。
    * コンストラクタ中で発生する例外は、MyNantokaApplicationExceptionのSubtypeとして例えばConstructExceptionなどを作成して、「アプリケーションの例外」の枠組みに含めます。これはコンストラクタから直接スローして良い例外です。全てのインスタンス生成はエントリポイントのtryブロック以下の処理の中で行われているはずなので、そのどこかの段階でリカバリーできないのであれば、他の異常終了と同様にエントリポイントのcatchブロックで扱うことができるようになります。

    「アプリケーションが落ちるほどの例外」という自分の表現がまず的を外していました。たとえ何か些細な例外が発生したとして、それを誰一人想定しておらず、エントリポイントまで到達してしまえば、それこそが「致命的な例外」となるわけで、例外を取り扱う際に「catchされるレイヤ」を区分してしまうという考え方自体が間違いだったということです。こちらは全て自分の説明の誤りでした。申し訳ないです。こちらも日々勉強させて頂いています……。

    キャンセル

  • 2017/01/26 19:48 編集

    回答ありがとうございます。
    なるほど、見えてきたような気がします。致命的な例外とは、例外が誰にも処理されなかった結果であるべきで、ある例外を投げるやつは、その例外がどのように扱われるかということについて知るべきではないということですね。(自分自身で捕捉するのでなければ。)
    例外の名前の件も納得しました。
    方針についてもよく理解できました。ご提案いただいた方針でコードを変更しようと思います。
    なかなか時間がとれず、コードの更新が遅れてしまい申し訳ありません。

    いえいえ、考える機会をいただだいたことで、いい経験になったと思います。
    例外の重大さが大きければ大きいほど、それを処理できるオブジェクトが限られ、結果的によりエントリポイントに近い位置で処理されることになる、ということですね。

    追記1:ひとまず例外設計とコンストラクタでの例外のところについてコードを変更いたしました。
       残りのIMapData周りのところについてもできるだけ早く更新いたします。
       
    追記2:もしよろしければ、並行ですみませんが、是非こちらの質問にも何かご意見をいただければと思います。
    -> https://teratail.com/questions/63555

    キャンセル

  • 2017/01/27 11:02

    意図が伝わったようで安心しています。理解の通りで、例外という異常事態が予想外であればあるほど、どんどん上の立場までエスカレーションしていくという、いわゆる企業業務のシステムそのものと近い構造となるわけです。
    例外処理のコードについては、現状で問題ないと思います。C#の例外システム自体が持つスタックトレースを捨ててしまっていることだけが気がかりですが、スタックトレースを何かしらの方法で出力する行為が要求仕様に沿えないのであれば、諦めるしかないでしょうね。loggerがトレースの出力を受け入れられる設計であればそれが一番良いのですが……。

    コメントの、リソースの取り扱いの方にも一言。
    業務フローが仕様として定められている以上は、残念ですがそれに従うしかありませんね。ただ、仕様としては現状のままだとしても、実際にリソースを使用するメソッドの中身のほうは、リソースを本当に一瞬だけ開くことを意識して記述することをおすすめします。メソッド全体にリソースへの参照が散らばっているとメンテナンス性がかなり落ちるので、リソースをオープンする場所(本当は常時オープンしてても、そこで開くものと仮定して)を一箇所、あるいは数カ所にまとめるように記述すると多分後々楽になります。

    別質問の方、確認してみます。何か気づきがあれば回答してみます、よろしくお願いします。

    キャンセル

+2

  • IGetData
    rawDataを(どこからか)取得する
    取得できないこともある

  • IParseData
    rawDataをパースする
    パースできないこともある

  • ISendData
    データを送る

  • Test
    全体の処理ルールを決める。

というわけで、結果に応じて何するか決めるのはTestクラスの責任

IGetDataはデータを取得するのが仕事、
データが取得できないのが例外的状況ならDataGetメソッドで例外投げるし、
そうでないなら例えば戻り値をEitherなりOptionで返す。
データが取得できなかったら何するかはこいつの仕事じゃない。

IParseDataも同様に、
パースするのがこいつの仕事であって、
パースした結果に応じて何するかはこいつの仕事ではない。


各インターフェースは処理が中断するかどうかをTuple.Item1に設定するようにしました。trueは正常終了,falseは異常終了,nullは中断しないことを示すものとします。

IGetDataはデータを取ってくるものですが、
こいつが「中断するかどうか」という全体の処理の流れに口を出すのは
いかがなものでしょうか?

あと「処理の結果はtrue/false/nullで表します」と言われても
作った人しか対応関係がわからないので、enumかタプルやめて例外を使ったほうがいいです。


ところでインターフェースの名前ですが
例にしても、

  • IDataGetter
  • IDataPaser
  • IDataSender

のほうが好きです。


  • ファイルオープンに失敗したら、決まった場所に決まったログを吐く。
  • ファイルオープン失敗時の動作は変更の可能性があるし、そうなっても容易に切り替えられないといけない。
  • データはファイル以外のところから取ってくるかもしれないし、そうなっても容易に切り替えられないといけない。
  • ファイル以外のところから取ってくる場合もやっぱり失敗するかもしれない。
  • ファイル以外のところから取ってきて失敗した場合の動作はファイルオープン失敗のときと同じとは限らない。

...

  • そうだ八百屋の会計システムを扱えるようにしなきゃ!

と、柔軟にしていくのもキリがないので

  • Done is better than perfect に逃げる。
  • YAGNI原則に逃げる。
  • SOLID原則を突き詰めて、関数型言語に逃げる。そして迷う。

としたいところです。

いや俺は逃げないと言う場合は...どうしましょうね。

とりあえずGetData()は失敗の可能性があるので何らかの方法で伝えなきゃいけないのでそうするとして、
(もちろんIDataGetのシグネチャは返し方によっては変える。
C#だとインターフェースで例外を投げることを強制できないし、
じゃあEither型とかSuccess型を実装して使うのも大変だしで悩ましいです。)

失敗時にどうするかを変更可能にしたいならそれも引数として渡さなければならないですね、
新しくインターフェースを増やすか、Actionで渡すか悩みどころですね。
統一感出すならインターフェース増やしますか。
それかもうIGetDataの責務を拡張するうまい説明か新しいインターフェース名を考えるかです。

投稿

編集

  • 回答の評価を上げる

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

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

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

  • 回答の評価を下げる

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

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

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

  • 2017/01/20 12:58

    たしかにそうですね。
    では、仕事の観点からこれらのインターフェースのシグネチャは変えないままとして、どのようにコードを変更したらよいでしょうか。
    例えばファイルがなければ正常終了、ファイルのオープンに失敗したらエラーログを出力して異常終了の場合、GetDataにおいて、ない場合は空を返し、Testでは、空なら正常終了したりGetDataの結果を受け取って終了するかを返す新しいインターフェースを考えたりするのがよいのでしょうか。
    また、異常終了の場合はどうしたらよいでしょうか。異常終了なら例外をスローするという方法はありますが、その場合誰が例外を捕捉してログを出力すればよいでしょうか。ファイルのオープンに失敗した場合のエラーログは一言一句厳密に決まっているため、Testの呼び出し元でcatchしたりはできません。例外を検知してログを出力するIGetDataのデコレーターを作成するくらいしか思いつきません。その場合、ひとつのインターフェースにつきひとつのエラーログのみが許可されることになります。

    インターフェースの名前などは実際はもう少し違いますが、業務など関係するのでぼかしてます。実際は名詞形になってます。

    キャンセル

  • 2017/01/20 14:05

    悩ましいですよね、回答に追記しました。

    キャンセル

0

悩まれている点がいまいち理解できていないかもしれないので、的外れでしたらすいません。

結果データと別に処理結果をEnumとかで受け取って分岐すれば2点とも解決しないでしょうか。
実装としてはTupleで2つもらうでも、out引数でもらうでもいいと思いますが。

RawData raw;
var res = _getter.TryGetData(out raw);

if (res == GetDataResult.FileNotFound)
{
    //正常終了
    return true;
}
if (res == GetDataResult.Error)
{
    //異常終了
    return false;
}
//処理継続

投稿

  • 回答の評価を上げる

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

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

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

  • 回答の評価を下げる

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

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

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

0

こんにちは。SOLIDの原則と噛んでいるかわかりませんが、例として

Ping.Sendメソッドの結果をPingReplyクラスで返す実装が、標準のフレームワークには存在していますね。
すなわち、Do メソッドは、結果用のクラスを生成して、そこにどのような処理となったかステータスを入れます。

また、データ変換に失敗することが普段から十分ありえる(織り込み済み)ならば、例外送出はせず結果用クラスに情報を入れて呼び出し側が判断、特別な対処をユーザーに要求したいほど滅多におきないことであれば例外を送出して処理を中断させる、と私ならばします。

ご参考 - http://dobon.net/vb/dotnet/internet/ping.html 

var ping = new System.Net.NetworkInformation.Ping();
var reply = ping.Send("www.example.com");
if (reply.Status == System.Net.NetworkInformation.IPStatus.Success)
{
 /// ...
}
else
{
 /// ...
}

なお、TestクラスのDoメソッドが、3つの処理を可能な限り全うさせることを責務としているのであれば、ログを書くなどは Do メソッドのなかでやってはまずいでしょう(単一責任の原則に反します)

投稿

編集

  • 回答の評価を上げる

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

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

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

  • 回答の評価を下げる

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

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

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

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

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

関連した質問

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

  • C#

    7109questions

    C#はマルチパラダイムプログラミング言語の1つで、命令形・宣言型・関数型・ジェネリック型・コンポーネント指向・オブジェクティブ指向のプログラミング開発すべてに対応しています。