1

私は、あまりうまく書かれておらず、リファクタリングしたいかなり複雑なロジックを含むいくつかのコードに取り組んでいます。トピックは、ルールの検証と潜在的な違反の報告です。残念ながら、クラスの設計はかなり奇妙であるため、いくつかの IEnumerable の課題に悩まされています。

簡単な例として、次のようなものがあります。

IEnumerable<RuleDefinition>
IEnumerable<Request>

どこ

public class RuleDefinition
{
    public RequestType ConcerningRequestType { get; set; }
    public int MinimumDistanceBetweenRequests { get; set; }
}

public class Request
{
    public int TimeIndex { get; set; }
    public RequestType TypeOfThisRequest { get; set; }
}

明らかに、リクエスト タイプが一致し、2 つのリクエスト間の間隔 (TimeIndex) が短すぎる場合、ルールに違反しています。今、私は抽出したい：

  • ルール違反がある場合 (これはかなり簡単です)
  • どの規則に違反しているか
  • どのリクエストがルールに違反しているか

したがって、私たちの場合、次のようなものを取得したいと思います。

public class Violation
{
    public RuleDefinition ViolatedRule { get; set; }
    public Request FirstRequest { get; set; }
    public Request SecondRequest { get; set; }
}

これはかなり単純な問題だと思いますが、読みやすく保守しやすいと言える解決策を思いつくことができません。私はさまざまなことを試しました..常に完全に面倒です（この例を実装しようとしましたが、ひどいです）

この場合に使用するアイデア、パターンはありますか? (Resharper は .SelectMany を正しく提案することがよくありますが、それではさらに読みにくくなります)

編集:これが私の長くて醜い実装です。;)

var ruleDefinitions = new List<RuleDefinition>
{ 
    new RuleDefinition { 
        ConcerningRequestType = RequestType.Exclusive, 
        MinimumDistanceBetweenRequests = 2} 
};
var requests = new List<Request>()
    {
        new Request { TimeIndex = 1, TypeOfThisRequest = RequestType.Normal },
        new Request { TimeIndex = 1, TypeOfThisRequest = RequestType.Normal },
        new Request { TimeIndex = 2, TypeOfThisRequest = RequestType.Normal },

        new Request { TimeIndex = 3, TypeOfThisRequest = RequestType.Exclusive },
        new Request { TimeIndex = 4, TypeOfThisRequest = RequestType.Exclusive },
    };

var violations = new List<Violation>();
foreach (var rule in ruleDefinitions)
{
    var requestsMatchingType = requests.Where(r => r.TypeOfThisRequest == rule.ConcerningRequestType);
    foreach (var firstRequest in requestsMatchingType)
    {
        var collidingRequest = requests.FirstOrDefault(secondRequest => 
            secondRequest.TimeIndex > firstRequest.TimeIndex &&
            Math.Abs(secondRequest.TimeIndex - firstRequest.TimeIndex) < rule.MinimumDistanceBetweenRequests);

        if (collidingRequest != null)
        {
            violations.Add(new Violation
                {
                    ViolatedRule = rule,
                    FirstRequest = firstRequest,
                    SecondRequest = collidingRequest
                });
        }
    }
}
Console.WriteLine("found {0} violations.", violations.Count());
Console.ReadKey();
4

2 に答える 2

2

これは単純な作業ではないため、最初に行うことは、ここで必要なものを確認するためのインターフェイスを定義することです。

interface IViolationFinder
{
    IEnumerable<Violation> Search(
        IEnumerable<RuleDefinition> ruleDefinitions, 
        IEnumerable<Request> requests);
}

これで、何を実装する必要があるかが明確にわかりました。あなたの検索ロジックは非常に複雑なので、単一の linq で表現する必要はないと思います。できますが、すべきではありません。内部にlinqが埋め込まれた2つのネストされたforeachループは非常に厄介で、linq自体ではきれいになるとは思いません。

必要なのは、実装内でより多くのメソッドを作成することです。可読性を高めていきます。したがって、単純な実装は次のようになります (これはあなたのものです)。

class ViolationFinder : IViolationFinder
{
    public IEnumerable<Violation> Search(IEnumerable<RuleDefinition> ruleDefinitions, IEnumerable<Request> requests)
    {
        var violations = new List<Violation>();
        foreach (var rule in ruleDefinitions)
        {
            var requestsMatchingType = requests.Where(r => r.TypeOfThisRequest == rule.ConcerningRequestType);
            foreach (var firstRequest in requestsMatchingType)
            {
                var collidingRequest = requests.FirstOrDefault(secondRequest =>
                    secondRequest.TimeIndex > firstRequest.TimeIndex &&
                    Math.Abs(secondRequest.TimeIndex - firstRequest.TimeIndex) < rule.MinimumDistanceBetweenRequests);

                if (collidingRequest != null)
                {
                    violations.Add(new Violation
                    {
                        ViolatedRule = rule,
                        FirstRequest = firstRequest,
                        SecondRequest = collidingRequest
                    });
                }
            }
        }

        return violations;
    }
}

これでリファクタリングを開始できます。1 つの方法で考えるのではなく、最も明白な部分を抽出しましょう。

class ViolationFinder : IViolationFinder
{
    public IEnumerable<Violation> Search(IEnumerable<RuleDefinition> ruleDefinitions, IEnumerable<Request> requests)
    {
        var violations = new List<Violation>();
        foreach (RuleDefinition rule in ruleDefinitions)
        {
            IEnumerable<Request> requestsMatchingType = requests.Where(r => r.TypeOfThisRequest == rule.ConcerningRequestType);
            violations.AddRange(
                FindViolationsInRequests(requestsMatchingType, requests, rule));
        }

        return violations;
    }

    private IEnumerable<Violation> FindViolationsInRequests(
        IEnumerable<Request> matchingRequests,
        IEnumerable<Request> allRequest,
        RuleDefinition rule)
    {
        foreach (Request firstRequest in matchingRequests)
        {
            var collidingRequest = allRequest.FirstOrDefault(secondRequest =>
                secondRequest.TimeIndex > firstRequest.TimeIndex &&
                Math.Abs(secondRequest.TimeIndex - firstRequest.TimeIndex) < rule.MinimumDistanceBetweenRequests);

            if (collidingRequest != null)
            {
                yield return new Violation
                {
                    ViolatedRule = rule,
                    FirstRequest = firstRequest,
                    SecondRequest = collidingRequest
                };
            }
        }
    }
}

検索はほぼクリーンですが、FindViolationsInRequests がすべてのリクエストとルールを取得するため、フィルター処理されたリクエスト リストを渡すことはまったく役に立ちません。したがって、これを行います。

class ViolationFinder : IViolationFinder
{
    public IEnumerable<Violation> Search(IEnumerable<RuleDefinition> ruleDefinitions, IEnumerable<Request> requests)
    {
        var violations = new List<Violation>();
        foreach (RuleDefinition rule in ruleDefinitions)
        {
            violations.AddRange(FindViolationsInRequests(requests, rule));
        }

        return violations;
    }

    private IEnumerable<Violation> FindViolationsInRequests(
        IEnumerable<Request> allRequest,
        RuleDefinition rule)
    {
        foreach (Request firstRequest in FindMatchingRequests(allRequest, rule))
        {
            var collidingRequest = allRequest.FirstOrDefault(secondRequest =>
                secondRequest.TimeIndex > firstRequest.TimeIndex &&
                Math.Abs(secondRequest.TimeIndex - firstRequest.TimeIndex) < rule.MinimumDistanceBetweenRequests);

            if (collidingRequest != null)
            {
                yield return new Violation
                {
                    ViolatedRule = rule,
                    FirstRequest = firstRequest,
                    SecondRequest = collidingRequest
                };
            }
        }
    }

    private IEnumerable<Request> FindMatchingRequests(IEnumerable<Request> requests, RuleDefinition rule)
    {
        return requests.Where(r => r.TypeOfThisRequest == rule.ConcerningRequestType);
    }
}

次のことは、

    var collidingRequest = allRequest.FirstOrDefault(secondRequest =>
        secondRequest.TimeIndex > firstRequest.TimeIndex &&
        Math.Abs(secondRequest.TimeIndex - firstRequest.TimeIndex) < rule.MinimumDistanceBetweenRequests);

いくつかの方法を作成するのに十分複雑です：

class ViolationFinder : IViolationFinder
{
    public IEnumerable<Violation> Search(IEnumerable<RuleDefinition> ruleDefinitions, IEnumerable<Request> requests)
    {
        var violations = new List<Violation>();

        foreach (RuleDefinition rule in ruleDefinitions)
        {
            violations.AddRange(FindViolationsInRequests(requests, rule));
        }

        return violations;
    }

    private IEnumerable<Violation> FindViolationsInRequests(
        IEnumerable<Request> allRequest,
        RuleDefinition rule)
    {
        foreach (Request firstRequest in FindMatchingRequests(allRequest, rule))
        {

            Request collidingRequest = FindCollidingRequest(allRequest, firstRequest, rule.MinimumDistanceBetweenRequests);

            if (collidingRequest != null)
            {
                yield return new Violation
                {
                    ViolatedRule = rule,
                    FirstRequest = firstRequest,
                    SecondRequest = collidingRequest
                };
            }
        }
    }

    private IEnumerable<Request> FindMatchingRequests(IEnumerable<Request> requests, RuleDefinition rule)
    {
        return requests.Where(r => r.TypeOfThisRequest == rule.ConcerningRequestType);
    }

    private Request FindCollidingRequest(IEnumerable<Request> requests, Request firstRequest, int minimumDistanceBetweenRequests)
    {
        return requests.FirstOrDefault(secondRequest => IsCollidingRequest(firstRequest, secondRequest, minimumDistanceBetweenRequests));
    }

    private bool IsCollidingRequest(Request firstRequest, Request secondRequest, int minimumDistanceBetweenRequests)
    {
        return secondRequest.TimeIndex > firstRequest.TimeIndex &&
               Math.Abs(secondRequest.TimeIndex - firstRequest.TimeIndex) < minimumDistanceBetweenRequests;
    }
}

よし、きれいになってきました。すべてのメソッドの目的はほとんど簡単にわかります。もう少し作業をすると、次のようになります。

class ViolationFinder : IViolationFinder
{
    public IEnumerable<Violation> Search(IEnumerable<RuleDefinition> ruleDefinitions, IEnumerable<Request> requests)
    {
        List<Request> requestList = requests.ToList();
        return ruleDefinitions.SelectMany(rule => FindViolationsInRequests(requestList, rule));
    }

    private IEnumerable<Violation> FindViolationsInRequests(IEnumerable<Request> allRequest, RuleDefinition rule)
    {
        return FindMatchingRequests(allRequest, rule)
                .Select(firstRequest => FindSingleViolation(allRequest, firstRequest, rule))
                .Where(violation => violation != null);
    }

    private Violation FindSingleViolation(IEnumerable<Request> allRequest, Request request, RuleDefinition rule)
    {
        Request collidingRequest = FindCollidingRequest(allRequest, request, rule.MinimumDistanceBetweenRequests);

        if (collidingRequest != null)
        {
            return new Violation
            {
                ViolatedRule = rule,
                FirstRequest = request,
                SecondRequest = collidingRequest
            };
        }

        return null;
    }

    private IEnumerable<Request> FindMatchingRequests(IEnumerable<Request> requests, RuleDefinition rule)
    {
        return requests.Where(r => r.TypeOfThisRequest == rule.ConcerningRequestType);
    }

    private Request FindCollidingRequest(IEnumerable<Request> requests, Request firstRequest, int minimumDistanceBetweenRequests)
    {
        return requests.FirstOrDefault(secondRequest => IsCollidingRequest(firstRequest, secondRequest, minimumDistanceBetweenRequests));
    }

    private bool IsCollidingRequest(Request firstRequest, Request secondRequest, int minimumDistanceBetweenRequests)
    {
        return secondRequest.TimeIndex > firstRequest.TimeIndex &&
               Math.Abs(secondRequest.TimeIndex - firstRequest.TimeIndex) < minimumDistanceBetweenRequests;
    }
}

メソッドにも単一責任の原則が適用されることに注意してください。Search メソッドを除いて、すべてが非公開実装の一部ですが、ご覧のとおり、各処理部分には名前付きのメソッドがあります。各メソッドには単一の責任があるため、実装をより簡単に読むことができます。

  • 検索（公開）
  • FindViolationsInRequests
  • FindSingleViolation
  • FindMatchingRequests
  • FindColidingRequest
  • IsCollidingRequest

これらは、この実装の単位です。

元の実装の単体テストを作成し、その後でリファクタリングを開始すると、リファクタリング プロセスはより安全になります。そうすれば、論理を破らないことが常にわかります。単体テストは、最初のバリエーション (完全なコードを Search メソッドに入れるとき) に対して記述すれば問題ありません。つまり、インターフェイスに対してです。

もう 1 つの小さいながらも重要な部分は次のとおりです。

public IEnumerable<Violation> Search(IEnumerable<RuleDefinition> ruleDefinitions, IEnumerable<Request> requests)
{
    List<Request> requestList = requests.ToList();
    return ruleDefinitions.SelectMany(rule => FindViolationsInRequests(requestList, rule));
}

項目からリストを作成するので、IEnumerable を複数回列挙するつもりはないと確信しています (特定の実装では問題が発生する可能性があります。IQueryable について考えてください)。

于 2013-03-22T18:03:26.817 に答える
0

If you are not opposed to using query expressions, then you can write your implementation as:

var violations = from rule in ruleDefinitions
                 join r1 in requests on rule.ConcerningRequestType equals r1.TypeOfThisRequest
                 join r2 in requests on rule.ConcerningRequestType equals r2.TypeOfThisRequest
                 where r1 != r2 &&
                       r2.TimeIndex > r1.TimeIndex &&
                       Math.Abs(r2.TimeIndex - r1.TimeIndex) < rule.MinimumDistanceBetweenRequests
                 select new Violation() { FirstRequest = r1, SecondRequest = r2, ViolatedRule = rule };
于 2013-03-22T18:50:21.677 に答える