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

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

ただいまの
回答率

90.01%

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

受付中

回答 4

投稿 編集

  • 評価
  • クリップ 5
  • VIEW 2,220

syogakusya

score 65

質問

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

// データを取得
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/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.01%
  • 質問をまとめることで、思考を整理して素早く解決
  • テンプレート機能で、簡単に質問をまとめられる

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