Jak wdrożyć zasadę DRY przy użyciu słowa kluczowego „using”?

23

Rozważ te metody:

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

Korzystanie z bloku jest takie samo i zostało tutaj powtórzone 3 razy (oczywiście ponad 100 razy w prawdziwej aplikacji). Jak można zaimplementować zasadę DRY (Don't Repeat Yourself) dla usingbloku? Czy w ogóle uważa się to za naruszenie zasady DRY?

Aktualizacja: Nie mówię o tym, co zostało zaimplementowane w usingbloku. Chodzi mi o to, że using (Entities entities = new Entities()). Ta linia jest powtarzana 100 razy lub więcej.

Saeed Neamati
źródło
2
jest to C #? Odpowiedź na twoje pytanie może zależeć od języka
David
Tak, David, przepraszam, że nie wspomniałem o moim języku. Jak może to wpłynąć na odpowiedź?
Saeed Neamati
niektóre języki mogą mieć specjalną składnię, która może pomóc ci uwzględnić trochę twojego kodu. Nie znam C #, ale myślę, że w Ruby można użyć bloków, aby uwzględnić część używaną.
David
Instrukcja using faktycznie zapewnia obsługę języka C # w celu zastosowania zasady DRY, aby uniknąć powtarzalnego kodowania podczas zarządzania usuwaniem zasobów za pomocą wzorca projektowego Dispose . To nie znaczy, że nie możemy znaleźć sposobów, aby sprawić, że rzeczy SUSZĄ! Osobiście uważam DRY za proces rekurencyjny.
John Tobler

Odpowiedzi:

24

Jednym z pomysłów byłoby owijanie go funkcją, która wymaga Func.

Coś takiego

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

Wtedy twój powyższy kod staje się

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

Zrobiłem Entitiesteż param typu, ponieważ zakładam, że masz więcej niż jeden typ, z którym to robisz. Jeśli nie, możesz go usunąć i po prostu użyć parametru type dla typu zwracanego.

Szczerze mówiąc, ten rodzaj kodu wcale nie poprawia czytelności. Z mojego doświadczenia wynika, że ​​bardziej współpracownicy Jr również mają z tym naprawdę ciężki czas.

Aktualizacja Niektóre dodatkowe odmiany pomocników, które możesz rozważyć

//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.
Potok
źródło
1
+1 Dobre rozwiązanie, choć nie rozwiązuje rzeczywistego problemu (nieuwzględnionego w pierwotnym pytaniu), tj. Wielu wystąpień Entities.
back2dos
@Doc Brown: Myślę, że pobiłem cię nazwą funkcji. Zostawiłem tę IEnumerablefunkcję na wypadek, gdyby były jakieś IEnumerablewłaściwości T, które dzwoniący chciał zwrócić, ale masz rację, to by to trochę wyczyściło. Być może IEnumerabledobrze byłoby mieć pomocnika zarówno dla singla, jak i dla wyników. To powiedziawszy, nadal myślę, że spowalnia rozpoznawanie tego, co robi kod, szczególnie dla kogoś, kto nie jest przyzwyczajony do używania wielu ogólnych i lambdas (np. Twoi współpracownicy, którzy NIE są na SO :))
Brook
+1 Myślę, że to podejście jest w porządku. A czytelność można poprawić. Na przykład wstaw „ToList” WithEntities, użyj Func<T,IEnumerable<K>>zamiast Func<T,K>i nadaj „WithEntities” lepszą nazwę (np. SelectEntities). I nie sądzę, aby „jednostki” musiały być tutaj ogólnym parametrem.
Doc Brown
1
Aby to zadziałało, musiałyby istnieć ograniczenia where T : IDisposable, new(), tak jak jest to usingwymagane IDisposabledo działania.
Anthony Pegram
1
@Anthony Pegram: Naprawiono, dziękuję! (to dostaję za kodowanie C # w oknie przeglądarki)
Brook
23

Dla mnie byłoby to tak, jakby martwić się o wielokrotne przepowiadanie tej samej kolekcji: to po prostu coś, co musisz zrobić. Jakakolwiek próba jego dalszego wyodrębnienia spowodowałaby, że kod byłby mniej czytelny.

Ben Hughes
źródło
Świetna analogia @ Ben. +1
Saeed Neamati
3
Nie zgadzam się, przepraszam. Przeczytaj komentarze OP dotyczące zakresu transakcji i zastanów się, co musisz zrobić, gdy napiszesz 500 razy tego rodzaju kod, a następnie zauważ, że będziesz musiał zmienić to samo 500 razy. Ten rodzaj powtarzania kodu może być poprawny tylko wtedy, gdy masz <10 z tych funkcji.
Doc Brown
Aha, i nie zapominając, że jeśli dla tej samej kolekcji więcej niż 10 razy w bardzo podobny sposób, z podobnie wyglądającym kodem wewnątrz każdego z was, zdecydowanie powinieneś pomyśleć o pewnym uogólnieniu.
Doc Brown
1
Dla mnie wygląda to tak, jakbyś robił trzy liniowiec w oneliner ... wciąż się powtarzasz.
Jim Wolff,
Jest zależny od kontekstu, jeśli na przykład foreachjest w bardzo dużej kolekcji lub logika w foreachpętli jest na przykład czasochłonna. Motto, które przyjąłem: nie ma obsesji, ale zawsze pamiętaj o swoim podejściu
Coops
9

Wygląda na to, że mylisz zasadę „Raz i tylko raz” z zasadą OSUSZANIA. Zasada DRY stanowi:

Każda wiedza musi mieć jedną, jednoznaczną, autorytatywną reprezentację w systemie.

Jednak zasada „raz i tylko raz” jest nieco inna.

Zasada [DRY] jest podobna do OnceAndOnlyOnce, ale ma inny cel. Dzięki OnceAndOnlyOnce zachęca się do refaktoryzacji w celu wyeliminowania powielonego kodu i funkcjonalności. Za pomocą DRY próbujesz zidentyfikować pojedyncze, ostateczne źródło każdej wiedzy wykorzystanej w twoim systemie, a następnie użyć tego źródła do wygenerowania odpowiednich instancji tej wiedzy (kod, dokumentacja, testy itp.).

Zasada DRY jest zwykle stosowana w kontekście rzeczywistej logiki, nie jest zbyteczna przy użyciu instrukcji:

Utrzymanie struktury programu DRY jest trudniejsze i ma niższą wartość. To reguły biznesowe, instrukcje if, formuły matematyczne i metadane powinny pojawiać się tylko w jednym miejscu. Rzeczy WET - strony HTML, nadmiarowe dane testowe, przecinki i ograniczniki {} - łatwo można zignorować, więc SUSZENIE ich jest mniej ważne.

Źródło

Phil
źródło
7

Nie widzę zastosowania usingtutaj:

Co powiesz na:

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

Lub nawet lepiej, ponieważ nie sądzę, że musisz za każdym razem tworzyć nowy obiekt.

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

Jeśli chodzi o naruszenie DRY: DRY nie ma zastosowania na tym poziomie. W rzeczywistości żadna zasada tak naprawdę nie działa, z wyjątkiem zasady czytelności. Próba zastosowania DRY na tym poziomie jest tak naprawdę tylko mikrooptymalizacją architektoniczną, która podobnie jak każda mikrooptymalizacja jest po prostu zrzucaniem rowerów i nie rozwiązuje żadnych problemów, ale nawet grozi wprowadzeniem nowych.
Z własnego doświadczenia wiem, że jeśli spróbujesz zmniejszyć nadmiarowość kodu na tym poziomie, wywrzesz negatywny wpływ na jakość kodu, zaciemniając to, co było naprawdę jasne i proste.

Edycja:
Ok. Tak więc problemem nie jest tak naprawdę instrukcja using, problemem jest zależność od obiektu, który tworzysz za każdym razem. Sugerowałbym wstrzyknięcie konstruktora:

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
źródło
2
Ale @ back2dos, istnieje wiele miejsc, w których używamy using (CustomTransaction transaction = new CustomTransaction())bloku kodu w naszym kodzie, aby zdefiniować zakres transakcji. Tego nie da się połączyć w jeden obiekt i w każdym miejscu, w którym chcesz skorzystać z transakcji, powinieneś napisać blok. Co teraz, jeśli chcesz zmienić typ tej transakcji z CustomTransactionna BuiltInTransactionponad 500 metod? Wydaje mi się, że jest to powtarzające się zadanie i przykład naruszenia zasady SUCHEGO podmiotu.
Saeed Neamati
3
Posługiwanie się tutaj „używaniem” zamyka kontekst danych, więc leniwe ładowanie nie jest możliwe poza tymi metodami.
Steven Striga
@ Saeed: To wtedy patrzysz na zastrzyk zależności. Wydaje się jednak, że jest to zupełnie odmienne od przypadku przedstawionego w pytaniu.
CVn
@Saeed: Post został zaktualizowany.
back2dos
@WeekendWarrior Czy using(w tym kontekście) jest jeszcze bardziej nieznaną „wygodną składnią”? Dlaczego tak przyjemnie jest używać =)
Coops
4

Nie tylko używanie jest duplikatem kodu (tak na marginesie, jest duplikatem kodu i faktycznie porównuje się z instrukcją try..catch..finally), ale także toList. Zmienilbym kod twojego kodu w ten sposób:

 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
źródło
3

Ponieważ nie ma tutaj żadnej logiki biznesowej z wyjątkiem ostatniej. Moim zdaniem nie jest to naprawdę SUCHE.

Ten ostatni nie zawiera DRY w bloku używającym, ale myślę, że klauzula where powinna się zmienić tam, gdzie kiedykolwiek była używana.

Jest to typowe zadanie generatorów kodu. Napisz i zakryj generator kodu i pozwól mu generować dla każdego typu.

arunmur
źródło
Nie @arunmur. Tu było nieporozumienie. Przez DRY miałem na myśli using (Entities entities = new Entities())blok. Mam na myśli, że ten wiersz kodu jest powtarzany 100 razy i jest powtarzany coraz więcej.
Saeed Neamati
OSUSZANIE wynika przede wszystkim z faktu, że musisz pisać przypadki testowe wiele razy, a błąd w jednym miejscu oznacza, że ​​musisz to naprawić w 100 miejscach. Użycie (Entities ...) jest zbyt prostym kodem, aby go złamać. Nie trzeba go dzielić ani umieszczać w innej klasie. Jeśli nadal nalegasz na uproszczenie. Sugerowałbym funkcję zwrotną Yeild z ruby.
arunmur
1
@arnumur - używanie nie zawsze jest zbyt łatwe do złamania. Często mam dobrą logikę, która określa, które opcje należy zastosować w kontekście danych. Bardzo możliwe, że niewłaściwy ciąg połączenia zostanie przekazany.
Morgan Herlocker
1
@ironcode - W takich sytuacjach sensowne jest zepchnięcie go do funkcji. Ale w tych przykładach trudno jest je złamać. Nawet jeśli się zepsuje, poprawka często znajduje się w definicji samej klasy Entities, którą należy objąć osobnym przypadkiem testowym.
arunmur
+1 @arunmur - Zgadzam się. Zwykle robię to sam. Piszę testy dla tej funkcji, ale wydaje się, że napisanie testu dla instrukcji using wydaje się nieco przesadne.
Morgan Herlocker
2

Ponieważ ciągle tworzysz i niszczysz ten sam obiekt jednorazowy, twoja klasa sama jest dobrym kandydatem do wdrożenia wzorca IDisposable.

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

To pozostawia tylko „używanie” podczas tworzenia instancji klasy. Jeśli nie chcesz, aby klasa była odpowiedzialna za usuwanie obiektów, możesz sprawić, że metody zaakceptują zależność jako argument:

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
źródło
1

Moja ulubiona odrobina niezrozumiałej magii!

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

Wrapistnieje tylko po to, aby wyodrębnić tę magię lub inną potrzebną magię. Nie jestem pewien, czy poleciłbym to przez cały czas, ale można z niego korzystać. „Lepszym” pomysłem byłoby użycie kontenera DI, takiego jak StructureMap, i po prostu objęcie klasy Entities kontekstem żądania, wstrzyknięcie go do kontrolera, a następnie pozwolenie mu zająć się cyklem życia bez konieczności kontrolera.

Travis
źródło
Parametry typu Func <IEnumerable <T>, Entities> są w niewłaściwej kolejności. (patrz moja odpowiedź, która ma w zasadzie to samo)
Brook
Cóż, wystarczająco łatwe do poprawienia. Nigdy nie pamiętam, jaka jest właściwa kolejność. Używam Funcwystarczająco, że powinienem.
Travis,