Wie implementiere ich das DRY-Prinzip, wenn ich das Schlüsselwort 'using' verwende?

23

Betrachten Sie diese Methoden:

public List<Employee> GetAllEmployees()
{
    using (Entities entities = new Entities())
    {
        return entities.Employees.ToList();
    }
}

public List<Job> GetAllJobs()
{
    using (Entities entities = new Entities())
    {
        return entities.Jobs.ToList();
    }
}

public List<Task> GetAllTasksOfTheJob(Job job)
{
    using (Entities entities = new Entities())
    {
        return entities.Tasks.Where(t => t.JobId == job.Id).ToList();
    }
}

Die Verwendung von Block ist dieselbe und wurde hier dreimal wiederholt (natürlich mehr als 100 Mal in der realen Anwendung). Wie ist es möglich, ein DRY-Prinzip (Don't Repeat Yourself) für einen usingBlock zu implementieren ? Gilt es überhaupt als Verstoß gegen den DRY-Grundsatz?

Update: Ich spreche nicht darüber, was innerhalb des usingBlocks implementiert wurde . Was ich hier eigentlich meine, ist das using (Entities entities = new Entities()). Diese Zeile wird mindestens 100 Mal wiederholt.

Saeed Neamati
quelle
2
ist das C #? Die Antwort auf Ihre Frage könnte sprachabhängig sein
David
Yeah @ David, sorry, dass ich meine Sprache nicht erwähnt habe. Wie kann es die Antwort beeinflussen?
Saeed Neamati
Einige Sprachen können eine bestimmte Syntax haben, die Ihnen dabei hilft, einen Teil Ihres Codes zu berücksichtigen. Ich kenne C # nicht, aber in Ruby könnten Sie Blöcke verwenden, um den using-Teil zu faktorisieren.
David
Die using-Anweisung bietet tatsächlich Unterstützung für die Sprache C #, um das DRY-Prinzip anzuwenden und wiederholte Codierungen zu vermeiden, während die Ressourcenentsorgung mit dem Dispose-Entwurfsmuster verwaltet wird . Das heißt nicht, dass wir keine Möglichkeiten finden können, Dinge trockener zu machen! Ich persönlich betrachte DRY als einen rekursiven Prozess.
John Tobler

Antworten:

24

Eine Idee wäre, es mit einer Funktion zu umschließen, die a annimmt Func.

Etwas wie das

public K UsingT<T,K>(Func<T,K> f) where T:IDisposable,new()
{
    using (T t = new T())
    {
        return f(t);
    }
}

Dann wird Ihr oben genannter Code

public List<Employee> GetAllEmployees()
{
    return UsingT<Entities,List<Employee>>(e=>e.Employees.ToList());
}

public List<Job> GetAllJobs()
{
    return UsingT<Entities,List<Job>>(e=>e.Jobs.ToList());
}

public List<Task> GetAllTasksOfTheJob(Job job)
{
    return UsingT<Entities,List<Task>>(e=>e.Tasks.Where(t => t.JobId == job.Id).ToList());
}

Ich habe auch Entitieseinen Typ-Parameter erstellt, da ich davon ausgehe, dass Sie mehr als einen Typ haben, mit dem Sie dies tun. Wenn dies nicht der Fall ist, können Sie es entfernen und einfach den type-Parameter für den Rückgabetyp verwenden.

Um ehrlich zu sein, trägt diese Art von Code überhaupt nicht zur Lesbarkeit bei. Nach meiner Erfahrung haben es auch die mehr Jr.-Mitarbeiter sehr schwer.

Aktualisieren Sie einige zusätzliche Variationen der Helfer, die Sie möglicherweise in Betracht ziehen

//forget the Entities type param
public T UsingEntities<T>(Func<Entities,T> f)
{
    using (Entities e = new Entities())
    {
        return f(e);
    }
}
//forget the Entities type param, and return an IList
public IList<T> ListFromEntities<T>(Func<Entities,IEnumerable<T>> f)
{
    using (Entities e = new Entities())
    {
        return f(e).ToList();
    }
}
//doing the .ToList() forces the results to enumerate before `e` gets disposed.
Bach
quelle
1
+1 Schöne Lösung, obwohl sie nicht das eigentliche Problem (nicht in der ursprünglichen Frage enthalten) anspricht, dh die vielen Vorkommen von Entities.
back2dos
@ Doc Brown: Ich denke, ich habe dich mit dem Funktionsnamen geschlagen. Ich habe IEnumerabledie Funktion für den Fall, dass es irgendwelche Nicht- IEnumerableEigenschaften von T gab, die der Aufrufer zurückgeben wollte, weggelassen, aber Sie haben Recht, es würde ein bisschen aufräumen. Vielleicht IEnumerablewäre es gut , einen Helfer für Single und Ergebnisse zu haben. Das heißt, ich denke immer noch, es verlangsamt die Erkennung dessen, was der Code tut, insbesondere für jemanden, der nicht daran gewöhnt ist, viele Generika und Lambdas zu verwenden (z. B. Ihre Kollegen, die NICHT auf SO sind :))
Brook,
+1 Ich denke, dieser Ansatz ist in Ordnung. Und die Lesbarkeit kann verbessert werden. Fügen Sie beispielsweise die "ToList" ein WithEntities, verwenden Sie Func<T,IEnumerable<K>>anstelle von Func<T,K>und geben Sie "WithEntities" einen besseren Namen (wie SelectEntities). Und ich denke nicht, dass "Entities" hier ein generischer Parameter sein muss.
Doc Brown
1
Damit dies funktioniert, müssten die Einschränkungen so sein where T : IDisposable, new(), wie usinges IDisposablefür die Arbeit erforderlich ist .
Anthony Pegram
1
@ Anthony Pegram: Behoben, danke! (das ist, was ich für die Codierung von C # in einem Browserfenster bekommen)
Brook
23

Für mich wäre das, als würde man sich mehrmals darum sorgen, dieselbe Sammlung zu durchsuchen: Es ist nur etwas, was man tun muss. Jeder Versuch, ihn weiter zu abstrahieren, würde den Code viel weniger lesbar machen.

Ben Hughes
quelle
Tolle Analogie @Ben. +1
Saeed Neamati
3
Ich stimme nicht zu, sorry. Lesen Sie die Kommentare des OP zum Transaktionsumfang und überlegen Sie, was Sie tun müssen, wenn Sie 500-mal so viel Code geschrieben haben. Beachten Sie dann, dass Sie das Gleiche 500-mal ändern müssen. Diese Art der Code-Wiederholung ist möglicherweise nur dann in Ordnung, wenn Sie <10 dieser Funktionen haben.
Doc Brown
Oh, und nicht zu vergessen, wenn Sie dieselbe Sammlung mehr als zehnmal auf sehr ähnliche Weise mit ähnlich aussehendem Code in Ihrer Sammlung verwenden, sollten Sie auf jeden Fall über eine Verallgemeinerung nachdenken.
Doc Brown
1
Für mich sieht es einfach so aus, als ob Sie aus einem Drei-Liner einen Einzeiler machen ... Sie wiederholen sich immer noch.
Jim Wolff
Es ist kontextabhängig, ob sich das foreachObjekt über einer sehr großen Sammlung befindet oder die Logik innerhalb der foreachSchleife beispielsweise zeitaufwändig ist. Ein Motto, das ich übernommen habe: Sei nicht besessen, sondern achte immer auf deine Vorgehensweise
Coops
9

Es hört sich so an, als würden Sie das Prinzip "Einmal und nur einmal" mit dem DRY-Prinzip verwechseln. Das DRY-Prinzip lautet:

Jedes Wissen muss eine einzige, eindeutige und maßgebliche Repräsentation innerhalb eines Systems haben.

Das Prinzip von einmal und nur einmal unterscheidet sich jedoch geringfügig.

Das Prinzip [DRY] ähnelt dem von OnceAndOnlyOnce, hat jedoch ein anderes Ziel. Mit OnceAndOnlyOnce werden Sie aufgefordert, Änderungen vorzunehmen, um doppelten Code und doppelte Funktionen zu beseitigen. Mit DRY versuchen Sie, die einzige, endgültige Quelle für jedes in Ihrem System verwendete Wissen zu identifizieren und diese Quelle dann zu verwenden, um anwendbare Instanzen dieses Wissens zu generieren (Code, Dokumentation, Tests usw.).

Das DRY-Prinzip wird normalerweise im Kontext der eigentlichen Logik verwendet, nicht so sehr redundant mit Anweisungen:

Das Beibehalten der Struktur eines Programms DRY ist schwieriger und hat einen niedrigeren Wert. Es sind die Geschäftsregeln, die if-Anweisungen, die mathematischen Formeln und die Metadaten, die an nur einer Stelle angezeigt werden sollen. Die WET-Inhalte - HTML-Seiten, redundante Testdaten, Kommas und Trennzeichen - können leicht ignoriert werden. Daher ist es weniger wichtig, sie zu trocknen.

Quelle

Phil
quelle
7

Ich kann die Verwendung von usinghier nicht erkennen:

Wie wäre es mit:

public List<Employee> GetAllEmployees() {
    return (new Entities()).Employees.ToList();
}
public List<Job> GetAllJobs() {
    return (new Entities()).Jobs.ToList();
}
public List<Task> GetAllTasksOfTheJob(Job job) {
    return (new Entities()).Tasks.Where(t => t.JobId == job.Id).ToList();
}

Oder noch besser, da ich nicht denke, dass Sie jedes Mal ein neues Objekt erstellen müssen.

private Entities entities = new Entities();//not sure C# allows for that kind of initialization, but you can do it in the constructor if needed

public List<Employee> GetAllEmployees() {
    return entities.Employees.ToList();
}
public List<Job> GetAllJobs() {
    return entities.Jobs.ToList();
}
public List<Task> GetAllTasksOfTheJob(Job job) {
    return entities.Tasks.Where(t => t.JobId == job.Id).ToList();
}

Was die Verletzung von DRY angeht: DRY gilt auf dieser Ebene nicht. Tatsächlich gibt es kein Prinzip, außer dem der Lesbarkeit. Der Versuch, DRY auf dieser Ebene anzuwenden, ist eigentlich nur eine architektonische Mikrooptimierung, die wie jede Mikrooptimierung nur einen Fahrradabwurf darstellt und keine Probleme löst, sondern sogar das Risiko birgt, neue einzuführen.
Aus eigener Erfahrung weiß ich, dass Sie, wenn Sie versuchen, die Code-Redundanz auf dieser Ebene zu verringern, die Code-Qualität negativ beeinflussen, indem Sie das verschleiern, was wirklich klar und einfach war.

Bearbeiten:
Ok. Das Problem ist also nicht die using-Anweisung, sondern die Abhängigkeit von dem Objekt, das Sie jedes Mal erstellen. Ich würde vorschlagen, einen Konstruktor zu injizieren:

private delegate Entities query();//this should be injected from the outside (upon construction for example)
public List<Employee> GetAllEmployees() {
    using (var entities = query()) {//AFAIK C# can infer the type here
        return entities.Employees.ToList();
    }
}
//... and so on
back2dos
quelle
2
Aber @ back2dos, es gibt viele Stellen, an denen wir Codeblöcke using (CustomTransaction transaction = new CustomTransaction())in unserem Code verwenden, um den Umfang einer Transaktion zu definieren. Das kann nicht zu einem einzigen Objekt gebündelt werden, und Sie sollten an jeder Stelle, an der Sie eine Transaktion verwenden möchten, einen Block schreiben. Was nun , wenn Sie von der Art der Transaktion zu ändern , CustomTransactionum BuiltInTransactionin mehr als 500 Methoden? Dies scheint mir eine sich wiederholende Aufgabe und ein Verstoß gegen das DRY-Prinzip zu sein.
Saeed Neamati
3
Wenn Sie hier "using" wählen, wird der Datenkontext geschlossen, sodass ein verzögertes Laden außerhalb dieser Methoden nicht möglich ist.
Steven Striga
@ Saeed: Das ist, wenn Sie in Abhängigkeitsinjektion suchen. Das scheint aber ganz anders zu sein als in der Frage angegeben.
ein CVn
@ Saeed: Beitrag aktualisiert.
back2dos
@WeekendWarrior Ist using(in diesem Zusammenhang) noch eine unbekannte "bequeme Syntax"? Warum ist es so schön zu benutzen =)
Coops
4

Es wird nicht nur doppelter Code verwendet (übrigens ist es doppelter Code und wird tatsächlich mit einer try..catch..finally-Anweisung verglichen), sondern auch die toList. Ich würde Ihren Code so umgestalten:

 public List<T> GetAll(Func<Entities, IEnumerable<T>> getter) {
    using (Entities entities = new Entities())
    {
        return getter().ToList();
    }
 }

public List<Employee> GetAllEmployees()
{
    return GetAll(e => e.Employees);
}

public List<Job> GetAllJobs()
{
    return GetAll(e => e.Jobs);
}

public List<Task> GetAllTasksOfTheJob(Job job)
{
    return GetAll(e => e.Tasks.Where(t => t.JobId == job.Id));
}
Cohen
quelle
3

Da es hier außer der letzten keine Geschäftslogik gibt. Meiner Meinung nach ist es nicht wirklich trocken.

Der letzte hat kein DRY im using-Block, aber ich denke, die where-Klausel sollte sich ändern, wo immer sie verwendet wird.

Dies ist ein typischer Job für Codegeneratoren. Schreiben und decken Sie den Codegenerator ab und lassen Sie ihn für jeden Typ generieren.

Arunmur
quelle
Nein @arunmur. Hier gab es ein Missverständnis. Mit TROCKEN meine ich using (Entities entities = new Entities())Block. Ich meine, diese Codezeile wird 100 Mal wiederholt und wird immer mehr wiederholt.
Saeed Neamati
DRY kommt hauptsächlich von der Tatsache, dass Sie Testfälle so oft schreiben müssen und ein Fehler an einer Stelle bedeutet, dass Sie ihn an 100 Stellen beheben müssen. Die Verwendung von (Entities ...) ist zu einfach, um einen Code zu knacken. Es muss nicht aufgeteilt oder in eine andere Klasse eingeteilt werden. Wenn Sie immer noch darauf bestehen, es zu vereinfachen. Ich würde eine Yeild-Rückruffunktion von Ruby vorschlagen.
Arunmur
1
@arnumur - Verwenden ist nicht immer zu einfach zu brechen. Ich habe oft ein gutes Stück Logik, das festlegt, welche Optionen im Datenkontext verwendet werden sollen. Es ist sehr
wahrscheinlich,
1
@ironcode - In solchen Situationen ist es sinnvoll, die Funktion zu deaktivieren. Aber in diesen Beispielen ist es ziemlich schwer zu brechen. Auch wenn dies nicht funktioniert, liegt der Fix häufig in der Definition der Entities-Klasse selbst, die mit einem separaten Testfall behandelt werden sollte.
Arunmur
+1 @ arunmur - ich stimme zu. Normalerweise mache ich das selbst. Ich schreibe Tests für diese Funktion, aber es scheint etwas übertrieben, einen Test für Ihre using-Anweisung zu schreiben.
Morgan Herlocker
2

Da Sie immer wieder dasselbe Einwegobjekt erstellen und zerstören, ist Ihre Klasse selbst ein guter Kandidat für die Implementierung des IDisposable-Musters.

class ThisClass : IDisposable
{
    protected virtual Entities Context { get; set; }

    protected virtual void Dispose( bool disposing )
    {
        if ( disposing && Context != null )
            Context.Dispose();
    }

    public void Dispose()
    {
        Dispose( true );
    }

    public ThisClass()
    {
        Context = new Entities();
    }

    public List<Employee> GetAllEmployees()
    {
        return Context.Employees.ToList();
    }

    public List<Job> GetAllJobs()
    {
        return Context.Jobs.ToList();
    }

    public List<Task> GetAllTasksOfTheJob(Job job)
    {
        return Context.Tasks.Where(t => t.JobId == job.Id).ToList();
    }
}

Das bedeutet, dass Sie nur das "using" benötigen, wenn Sie eine Instanz Ihrer Klasse erstellen. Wenn Sie nicht möchten, dass die Klasse für die Disposition der Objekte verantwortlich ist, können Sie die Methoden veranlassen, die Abhängigkeit als Argument zu akzeptieren:

public static List<Employee> GetAllEmployees( Entities entities )
{
    return entities.Employees.ToList();
}

public static List<Job> GetAllJobs( Entities entities )
{
    return entities.Jobs.ToList();
}

public static List<Task> GetAllTasksOfTheJob( Entities entities, Job job )
{
    return entities.Tasks.Where(t => t.JobId == job.Id).ToList();
}
Sean
quelle
1

Mein Lieblingsteil der unverständlichen Magie!

public class Blah
{
  IEnumerable<T> Wrap(Func<Entities, IEnumerable<T>> act)
  {
    using(var entities = new Entities()) { return act(entities); }
  }

  public List<Employee> GetAllEmployees()
  {
    return Wrap(e => e.Employees.ToList());
  }

  public List<Job> GetAllJobs()
  {
    return Wrap(e => e.Jobs.ToList());
  }

  public List<Task> GetAllTasksOfTheJob(Job job)
  {
    return Wrap(e => e.Tasks.Where(x ....).ToList());
  }
}

Wrapexistiert nur, um das heraus zu abstrahieren oder welche Magie auch immer du brauchst. Ich bin mir nicht sicher, ob ich das die ganze Zeit empfehlen würde, aber es ist möglich es zu benutzen. Die "bessere" Idee wäre, einen DI-Container wie StructureMap zu verwenden und die Entities-Klasse einfach auf den Anforderungskontext zu beschränken, in den Controller einzuspeisen und dann den Lebenszyklus zu erledigen, ohne dass der Controller dies tun muss.

Travis
quelle
Ihre Typparameter für Func <IEnumerable <T>, Entities> sind in der falschen Reihenfolge. (Siehe meine Antwort, die im Grunde das gleiche hat)
Brook
Gut einfach genug zu korrigieren. Ich erinnere mich nie, was die richtige Reihenfolge ist. Ich benutze Funcgenug, was ich sollte.
Travis