Comment implémenter le principe DRY lors de l'utilisation du mot-clé 'using'?

23

Considérez ces méthodes:

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

L'utilisation de block est la même et a été répétée 3 fois ici (bien sûr, plus de 100 fois dans l'application réelle). Comment est-il possible d'implémenter le principal DRY (Don't Repeat Yourself) pour le usingbloc? Est-ce considéré comme une violation du principe DRY?

Mise à jour: je ne parle pas de ce qui a été implémenté à l'intérieur du usingbloc. Ce que je veux dire ici, c'est le using (Entities entities = new Entities()). Cette ligne est répétée 100 fois ou plus.

Saeed Neamati
la source
2
est-ce C #? La réponse à votre question pourrait dépendre de la langue
David
Ouais @David, désolé de ne pas avoir mentionné ma langue. Comment cela peut-il affecter la réponse?
Saeed Neamati
certaines langues peuvent avoir une syntaxe particulière qui peut vous aider à factoriser un peu de votre code. Je ne connais pas C #, mais dans Ruby, je pense que vous pouvez utiliser des blocs pour factoriser la partie utilisatrice.
David
L' instruction using fournit en fait une prise en charge du langage C # pour appliquer le principe DRY afin d'éviter le codage répétitif tout en gérant l'élimination des ressources avec le modèle de conception Dispose . Cela ne signifie pas que nous ne pouvons pas trouver des moyens de rendre les choses plus SÈCHES! Personnellement, je pense que DRY est un processus récursif.
John Tobler

Réponses:

24

Une idée serait de l'envelopper avec une fonction qui prend a Func.

Quelque chose comme ça

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

Ensuite, votre code ci-dessus devient

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

J'ai également fait Entitiesun paramètre de type, car je suppose que vous avez plus d'un type avec lequel vous faites cela. Si ce n'est pas le cas, vous pouvez le supprimer et utiliser simplement le paramètre de type pour le type de retour.

Pour être honnête, ce type de code n'aide en rien la lisibilité. D'après mon expérience, plus de collègues Jr ont également de la difficulté à le faire.

Mise à jour Quelques variantes supplémentaires sur les aides que vous pourriez envisager

//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.
Ruisseau
la source
1
+1 Belle solution, même si elle ne résout pas le problème réel (non inclus dans la question d'origine), c'est-à-dire les nombreuses occurrences de Entities.
back2dos
@Doc Brown: Je pense que je vous ai battu avec le nom de la fonction. J'ai laissé de IEnumerablecôté la fonction au cas où il y aurait des non- IEnumerablepropriétés de T que l'appelant voulait renvoyer, mais vous avez raison, cela nettoierait un peu. Il IEnumerableserait peut-être bon d' avoir un assistant pour le single et les résultats. Cela dit, je pense toujours que cela ralentit la reconnaissance de ce que fait le code, en particulier pour quelqu'un qui n'est pas habitué à utiliser beaucoup de génériques et de lambdas (par exemple, vos collègues qui ne sont PAS sur SO :))
Brook
+1 Je pense que cette approche est bonne. Et la lisibilité peut être améliorée. Par exemple, mettez la "ToList" dans WithEntities, utilisez à la Func<T,IEnumerable<K>>place de Func<T,K>et donnez à "WithEntities" un meilleur nom (comme SelectEntities). Et je ne pense pas que "Entités" doive être un paramètre générique ici.
Doc Brown
1
Pour que cela fonctionne, les contraintes devraient être where T : IDisposable, new(), comme l' usingexige IDisposable, pour fonctionner.
Anthony Pegram
1
@Anthony Pegram: Corrigé, merci! (c'est ce que j'obtiens pour coder C # dans une fenêtre de navigateur)
Brook
23

Pour moi, cela reviendrait à s'inquiéter de devoir répéter plusieurs fois la même collection: c'est juste quelque chose que vous devez faire. Toute tentative de l'abréger rendrait le code beaucoup moins lisible.

Ben Hughes
la source
Grande analogie @Ben. +1
Saeed Neamati
3
Je ne suis pas d'accord, désolé. Lisez les commentaires du PO sur la portée de la transaction et pensez à ce que vous devez faire lorsque vous avez écrit 500 fois ce type de code, puis notez que vous devrez changer la même chose 500 fois. Ce type de répétition de code peut être correct uniquement si vous avez <10 de ces fonctions.
Doc Brown
Oh, et n'oublions pas, si vous pour-chacun la même collection plus de 10 fois de manière très similaire, avec un code similaire à l'intérieur de votre pour-chacun, vous devriez certainement penser à une généralisation.
Doc Brown
1
Pour moi, il semble que vous transformiez un trois lignes en un seul ... vous vous répétez toujours.
Jim Wolff
Cela dépend du contexte, si l' foreachest sur une très grande collection ou si la logique dans la foreachboucle prend du temps par exemple. Une devise que j'en suis venue à adopter: n'observez pas mais soyez toujours attentif à votre approche
Coops
9

On dirait que vous confondez le principe "Une seule et unique fois" avec le principe DRY. Le principe DRY stipule:

Chaque élément de connaissance doit avoir une représentation unique, non ambiguë et faisant autorité au sein d'un système.

Cependant, le principe Once and Only Once est légèrement différent.

Le principe [SEC] est similaire à OnceAndOnlyOnce, mais avec un objectif différent. Avec OnceAndOnlyOnce, vous êtes encouragé à refactoriser pour éliminer le code et les fonctionnalités en double. Avec DRY, vous essayez d'identifier la source unique et définitive de chaque élément de connaissance utilisé dans votre système, puis utilisez cette source pour générer des instances applicables de cette connaissance (code, documentation, tests, etc.).

Le principe DRY est généralement utilisé dans le contexte d'une logique réelle, pas tellement redondant à l'aide d'instructions:

Garder la structure d'un programme SEC est plus difficile et de moindre valeur. Ce sont les règles métier, les instructions if, les formules mathématiques et les métadonnées qui ne doivent apparaître qu'à un seul endroit. Les éléments WET - pages HTML, données de test redondantes, virgules et délimiteurs {} - sont tous faciles à ignorer, il est donc moins important de les sécher.

La source

Phil
la source
7

Je ne vois pas l'utilisation d' usingici:

Que diriez-vous:

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

Ou encore mieux, car je ne pense pas que vous ayez besoin de créer un nouvel objet à chaque fois.

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 ce qui concerne la violation de DRY: DRY ne s'applique pas à ce niveau. En fait, aucun principe ne le fait vraiment, sauf celui de la lisibilité. Essayer d'appliquer DRY à ce niveau n'est en réalité que de la micro-optimisation architecturale, qui, comme toute micro-optimisation, n'est qu'un délestage de vélo et ne résout aucun problème, mais risque même d'en introduire de nouveaux.
D'après ma propre expérience, je sais que si vous essayez de réduire la redondance du code à ce niveau, vous créez un impact négatif sur la qualité du code, en obscurcissant ce qui était vraiment clair et simple.

Edit:
Ok. Ainsi, le problème n'est pas réellement l'instruction using, le problème est la dépendance à l'objet que vous créez à chaque fois. Je suggère d'injecter un constructeur:

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
la source
2
Mais @ back2dos, il existe de nombreux endroits où nous utilisons le using (CustomTransaction transaction = new CustomTransaction())bloc de code dans notre code pour définir la portée d'une transaction. Cela ne peut pas être regroupé en un seul objet et à chaque endroit où vous souhaitez utiliser une transaction, vous devez écrire un bloc. Maintenant, que se passe-t-il si vous souhaitez modifier le type de cette transaction de CustomTransactionà BuiltInTransactionplus de 500 méthodes? Cela me semble être une tâche répétitive et un exemple de violation du principal DRY.
Saeed Neamati
3
Le fait d '"utiliser" ici ferme le contexte de données, de sorte qu'un chargement paresseux n'est pas possible en dehors de ces méthodes.
Steven Striga
@Saeed: C'est à ce moment que vous examinez l'injection de dépendances. Mais cela semble être tout à fait différent du cas indiqué dans la question.
un CVn
@Saeed: Post mis à jour.
back2dos
@WeekendWarrior Est-ce que using(dans ce contexte) est une "syntaxe pratique" plus inconnue? Pourquoi c'est si agréable à utiliser =)
Coops
4

Non seulement l'utilisation est du code en double (d'ailleurs, c'est du code en double et se compare en fait à une instruction try..catch..finally) mais aussi la toList. Je voudrais refactoriser votre code comme ceci:

 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
la source
3

Puisqu'il n'y a ici aucune logique commerciale, à l'exception de la dernière. Ce n'est pas vraiment SEC, à mon avis.

Le dernier n'a pas de DRY dans le bloc using mais je suppose que la clause where devrait changer où qu'elle soit utilisée.

Il s'agit d'un travail typique pour les générateurs de code. Écrire et couvrir le générateur de code et le laisser générer pour chaque type.

arunmur
la source
Non @arunmur. Il y a eu un malentendu ici. Par SEC, je voulais dire using (Entities entities = new Entities())bloc. Je veux dire, cette ligne de code est répétée 100 fois et se répète de plus en plus.
Saeed Neamati
DRY vient principalement du fait que vous devez écrire des cas de test autant de fois et qu'un bogue à un endroit signifie que vous devez le corriger à 100 endroits. L'utilisation de (Entities ...) est un code trop simple à casser. Il n'a pas besoin d'être divisé ou placé dans une autre classe. Si vous insistez toujours pour la simplifier. Je suggérerais une fonction de rappel Yeild à partir de ruby.
arunmur
1
@arnumur - utiliser n'est pas toujours trop simple à casser. J'ai souvent une bonne logique qui détermine les options à utiliser dans le contexte des données. Il est très possible que la mauvaise chaîne de connexion soit transmise.
Morgan Herlocker
1
@ironcode - Dans ces situations, il est logique de le pousser vers une fonction. Mais dans ces exemples, c'est assez difficile à casser. Même si cela ne fonctionne pas, le correctif se trouve souvent dans la définition de la classe Entities elle-même, qui doit être couverte par un cas de test distinct.
arunmur
+1 @arunmur - Je suis d'accord. Je le fais habituellement moi-même. J'écris des tests pour cette fonction, mais il semble un peu exagéré d'écrire un test pour votre instruction using.
Morgan Herlocker
2

Puisque vous créez et détruisez le même objet jetable encore et encore, votre classe est elle-même un bon candidat pour implémenter le modèle 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();
    }
}

Cela vous laisse seulement besoin de «utiliser» lors de la création d'une instance de votre classe. Si vous ne voulez pas que la classe soit responsable de l'élimination des objets, vous pouvez faire en sorte que les méthodes acceptent la dépendance comme 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
la source
1

Mon morceau préféré de magie incompréhensible!

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

Wrapn'existe que pour en faire abstraction ou quelle que soit la magie dont vous avez besoin. Je ne suis pas sûr de le recommander tout le temps, mais il est possible de l'utiliser. La "meilleure" idée serait d'utiliser un conteneur DI, comme StructureMap, et d'étendre simplement la classe Entities au contexte de la demande, de l'injecter dans le contrôleur, puis de le laisser s'occuper du cycle de vie sans que votre contrôleur n'en ait besoin.

Travis
la source
Vos paramètres de type pour Func <IEnumerable <T>, Entities> sont dans le mauvais ordre. (voir ma réponse qui a essentiellement la même chose)
Brook
Eh bien, assez facile à corriger. Je ne me souviens jamais du bon ordre. J'en utilise Funcsuffisamment.
Travis