¿Cómo implementar el principio DRY cuando se usa la palabra clave 'using'?

23

Considere estos métodos:

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

Usar el bloque es igual y se ha repetido 3 veces aquí (por supuesto, más de 100 veces en la aplicación real). ¿Cómo es posible implementar el DRY (Don't Repeat Yourself) principal para el usingbloque? ¿Se considera una violación del director DRY?

Actualización: no estoy hablando de lo que se ha implementado dentro del usingbloque. Lo que realmente quiero decir aquí, es el using (Entities entities = new Entities()). Esta línea se repite 100 veces o más.

Saeed Neamati
fuente
2
¿Es esto C #? La respuesta a su pregunta podría depender del idioma
David
Sí @David, lamento no haber mencionado mi idioma. ¿Cómo puede afectar la respuesta?
Saeed Neamati
algunos idiomas pueden tener una sintaxis particular que puede ayudarlo a factorizar un poco de su código. No sé C #, pero en Ruby, creo que podrías usar bloques para factorizar la parte que usa.
David
La declaración de uso en realidad proporciona soporte de lenguaje C # para aplicar el principio DRY para ayudar a evitar la codificación repetitiva mientras se administra la eliminación de recursos con el patrón de diseño Dispose . ¡Eso no significa que no podamos encontrar maneras de hacer las cosas más SECAS! Personalmente, pienso en DRY como un proceso recursivo.
John Tobler

Respuestas:

24

Una idea sería envolverlo con una función que tome a Func.

Algo como esto

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

Entonces su código anterior se convierte en

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

También hice Entitiesun parámetro de tipo, porque supongo que tienes más de un tipo con el que estás haciendo esto. Si no lo está, puede eliminarlo y simplemente usar el tipo param para el tipo de retorno.

Para ser sincero, este tipo de código no ayuda en absoluto a la legibilidad. En mi experiencia, los compañeros de trabajo de Jr también lo pasan muy mal.

Actualización Algunas variaciones adicionales sobre los ayudantes que podría considerar

//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.
Arroyo
fuente
1
+1 Buena solución, aunque no aborda el problema real (no incluido en la pregunta original), es decir, las muchas ocurrencias de Entities.
back2dos
@ Doc Brown: Creo que te gané con el nombre de la función. Dejé IEnumerablefuera de la función en caso de que hubiera no IEnumerablepropiedades de T que la persona que llama quería que se devolviera, pero tiene razón, lo limpiaría un poco. Quizás tener un ayudante tanto para Single como para IEnumerableresultados sería bueno. Dicho esto, todavía creo que ralentiza el reconocimiento de lo que hace el código, especialmente para alguien que no está acostumbrado a usar muchos genéricos y lambdas (por ejemplo, sus compañeros de trabajo que NO están en SO :))
Brook,
+1 Creo que este enfoque está bien. Y la legibilidad se puede mejorar. Por ejemplo, coloque "ToList" en WithEntities, use en Func<T,IEnumerable<K>>lugar de Func<T,K>y asigne a "WithEntities" un nombre mejor (como SelectEntities). Y no creo que "Entidades" deba ser un parámetro genérico aquí.
Doc Brown
1
Para que esto funcione, las restricciones tendrían que ser where T : IDisposable, new(), como se usingrequiere IDisposablepara trabajar.
Anthony Pegram el
1
@ Anthony Pegram: Corregido, ¡gracias! (eso es lo que obtengo por codificar C # en una ventana del navegador)
Brook
23

Para mí, esto sería como preocuparse por predecir la misma colección varias veces: es algo que debe hacer. Cualquier intento de abstraerlo aún más hará que el código sea mucho menos legible.

Ben Hughes
fuente
Gran analogía @Ben. +1
Saeed Neamati
3
No estoy de acuerdo, lo siento. Lea los comentarios del OP sobre el alcance de la transacción y piense en lo que tiene que hacer cuando ha escrito 500 veces ese tipo de código, y luego observe que tendrá que cambiar lo mismo 500 veces. Este tipo de repetición de código puede estar bien solo si tiene <10 de esas funciones.
Doc Brown
Ah, y no lo olvides, si formas la misma colección más de 10 veces de una manera muy similar, con un código similar dentro de tu para cada uno, definitivamente deberías pensar en alguna generalización.
Doc Brown
1
Para mí, parece que estás convirtiendo un revestimiento de tres en uno ... todavía te estás repitiendo.
Jim Wolff
Depende del contexto, por ejemplo , si se foreachtrata de una colección muy grande o si la lógica dentro del foreachciclo consume mucho tiempo. Un lema que he llegado a adoptar: no te obsesiones, pero siempre ten en cuenta tu enfoque
Coops
9

Parece que está confundiendo el principio "Once and Only Once" con el principio DRY. El principio DRY establece:

Cada conocimiento debe tener una representación autoritaria, inequívoca y única dentro de un sistema.

Sin embargo, el principio Once and Only Once es ligeramente diferente.

El principio [DRY] es similar a OnceAndOnlyOnce, pero con un objetivo diferente. Con OnceAndOnlyOnce, se le recomienda refactorizar para eliminar el código y la funcionalidad duplicados. Con DRY, intenta identificar la fuente única y definitiva de cada conocimiento utilizado en su sistema, y ​​luego usa esa fuente para generar instancias aplicables de ese conocimiento (código, documentación, pruebas, etc.).

El principio DRY generalmente se usa en el contexto de la lógica real, no tanto redundante usando declaraciones:

Mantener la estructura de un programa DRY es más difícil y de menor valor. Son las reglas de negocio, las declaraciones if, las fórmulas matemáticas y los metadatos que deberían aparecer en un solo lugar. Las cosas WET (páginas HTML, datos de prueba redundantes, comas y {} delimitadores) son fáciles de ignorar, por lo que SECARlas es menos importante.

Fuente

Phil
fuente
7

No veo el uso de usingaquí:

Qué tal si:

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

O incluso mejor, ya que no creo que necesite crear un nuevo objeto cada vez.

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

En cuanto a violar DRY: DRY no se aplica en este nivel. De hecho, ningún principio realmente lo hace, excepto el de legibilidad. Intentar aplicar DRY a ese nivel es realmente una microoptimización arquitectónica, que al igual que todas las microoptimizaciones es solo arrojar bicicletas y no resuelve ningún problema, pero incluso se arriesga a introducir otras nuevas.
Según mi propia experiencia, sé que si intentas reducir la redundancia de código a ese nivel, creas un impacto negativo en la calidad del código, al ofuscar lo que era realmente claro y simple.

Editar:
Ok. Entonces, el problema no es en realidad la declaración de uso, el problema es la dependencia del objeto que crea cada vez. Sugeriría inyectar un constructor:

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
fuente
2
Pero @ back2dos, hay muchos lugares donde usamos using (CustomTransaction transaction = new CustomTransaction())bloque de código en nuestro código para definir el alcance de una transacción. Eso no se puede agrupar en un solo objeto y en cada lugar donde desee usar una transacción, debe escribir un bloque. Ahora, ¿qué pasa si desea cambiar el tipo de esa transacción CustomTransactiona BuiltInTransactionmás de 500 métodos? Esto me parece una tarea repetitiva y un ejemplo de incumplimiento del director DRY.
Saeed Neamati
3
Tener "usar" aquí cierra el contexto de datos, por lo que no es posible la carga diferida fuera de estos métodos.
Steven Striga
@Saeed: Ahí es cuando analizas la inyección de dependencia. Pero eso parece ser muy diferente del caso como se indica en la pregunta.
un CVn
@Saeed: Publicación actualizada.
back2dos
@WeekendWarrior ¿Es using(en este contexto) una "sintaxis conveniente" más desconocida? Por qué es tan agradable de usar =)
Coops
4

No solo usar es código duplicado (por cierto, es código duplicado y en realidad se compara con una declaración try..catch..finally) sino también toList. Refactorizaría su código así:

 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
fuente
3

Dado que no hay ninguna lógica de negocios de ningún tipo aquí, excepto la última. No es realmente SECO, en mi opinión.

El último no tiene DRY en el bloque de uso, pero supongo que la cláusula where debería cambiar donde sea que se haya usado.

Este es un trabajo típico para los generadores de código. Escriba y cubra el generador de código y déjelo generar para cada tipo.

Arunmur
fuente
No @arunmur. Hubo un malentendido aquí. Por SECO, me refería a using (Entities entities = new Entities())bloquear. Quiero decir, esta línea de código se repite 100 veces y se repite cada vez más.
Saeed Neamati
DRY proviene principalmente del hecho de que necesita escribir casos de prueba muchas veces y un error en un lugar significa que tiene que arreglarlo en 100 lugares. El uso de (Entidades ...) es un código demasiado simple para romperlo. No necesita ser dividido o puesto en otra clase. Si aún insistes en simplificarlo. Sugeriría una función de devolución de llamada de Yeild de ruby.
Arunmur
1
@arnumur: el uso no siempre es demasiado fácil de romper. A menudo tengo una buena lógica que determina qué opciones usar en el contexto de datos. Es muy posible que se pase la cadena de conexión incorrecta.
Morgan Herlocker
1
@ironcode: en esas situaciones, tiene sentido llevarlo a una función. Pero en estos ejemplos es bastante difícil de romper. Incluso si se rompe, la solución suele estar en la definición de la propia clase Entidades, que debería cubrirse con un caso de prueba separado.
Arunmur
+1 @arunmur: estoy de acuerdo. Usualmente lo hago yo mismo. Escribo pruebas para esa función, pero parece un poco exagerado escribir una prueba para su declaración de uso.
Morgan Herlocker
2

Como está creando y destruyendo el mismo objeto desechable una y otra vez, su clase es en sí misma un buen candidato para implementar el patrón 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();
    }
}

Esto te deja solo necesitando el "uso" al crear una instancia de tu clase. Si no desea que la clase sea responsable de deshacerse de los objetos, puede hacer que los métodos acepten la dependencia como argumento:

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
fuente
1

¡Mi parte favorita de magia incomprensible!

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

Wrapexiste solo para abstraer eso o cualquier magia que necesites. No estoy seguro de recomendarlo todo el tiempo, pero es posible usarlo. La idea "mejor" sería usar un contenedor DI, como StructureMap, y solo establecer el alcance de la clase Entities en el contexto de la solicitud, inyectarlo en el controlador y luego dejar que se ocupe del ciclo de vida sin que su controlador lo necesite.

Travis
fuente
Sus parámetros de tipo para Func <IEnumerable <T>, Entities> están en el orden incorrecto. (vea mi respuesta que tiene básicamente lo mismo)
Brook
Bueno, lo suficientemente fácil de corregir. Nunca recuerdo cuál es el orden correcto. Yo uso lo Funcsuficiente como debería.
Travis