Dois-je souffrir d'une surutilisation de l'encapsulation?

J'ai remarqué quelque chose dans mon code dans divers projets qui me semble être une odeur de code et quelque chose de mal à faire, mais je ne peux pas y faire face.

En essayant d'écrire du "code propre", j'ai tendance à sur-utiliser des méthodes privées afin de rendre mon code plus facile à lire. Le problème est que le code est en effet plus propre mais il est aussi plus difficile à tester (ouais je sais que je peux tester des méthodes privées ...) et en général cela me semble une mauvaise habitude.

Voici un exemple de classe qui lit certaines données d'un fichier .csv et renvoie un groupe de clients (un autre objet avec divers champs et attributs).

public class GroupOfCustomersImporter {
    //... Call fields ....
    public GroupOfCustomersImporter(String filePath) {
        this.filePath = filePath;
        customers = new HashSet<Customer>();
        createCSVReader();
        read();
        constructTTRP_Instance();
    }

    private void createCSVReader() {
        //....
    }

    private void read() {
        //.... Reades the file and initializes the class attributes
    }

    private void readFirstLine(String[] inputLine) {
        //.... Method used by the read() method
    }

    private void readSecondLine(String[] inputLine) {
        //.... Method used by the read() method
    }

    private void readCustomerLine(String[] inputLine) { 
        //.... Method used by the read() method
    }

    private void constructGroupOfCustomers() {
        //this.groupOfCustomers = new GroupOfCustomers(**attributes of the class**);
    }

    public GroupOfCustomers getConstructedGroupOfCustomers() {
        return this.GroupOfCustomers;
    }
}

Comme vous pouvez le voir, la classe n'a qu'un constructeur qui appelle des méthodes privées pour faire le travail, je sais que ce n'est pas une bonne pratique en général, mais je préfère encapsuler toutes les fonctionnalités de la classe au lieu de rendre les méthodes publiques, auquel cas un client doit travailler de cette façon:

GroupOfCustomersImporter importer = new GroupOfCustomersImporter(filepath)
importer.createCSVReader();
read();
GroupOfCustomer group = constructGoupOfCustomerInstance();

Je préfère cela parce que je ne veux pas mettre de lignes de code inutiles dans le code côté client dérangeant la classe client avec des détails d'implémentation.

Alors, est-ce vraiment une mauvaise habitude? Si oui, comment puis-je l'éviter? Veuillez noter que ce qui précède n'est qu'un exemple simple. Imaginez la même situation se produisant dans quelque chose d'un peu plus complexe.

Je pense que vous êtes réellement sur la bonne voie en ce qui concerne le fait de vouloir cacher les détails de l'implémentation au client. Vous souhaitez concevoir votre classe de sorte que l'interface que le client voit soit l'API la plus simple et la plus concise que vous puissiez trouver. Non seulement le client ne sera pas "dérangé" par les détails d'implémentation, mais vous serez également libre de refactoriser le code sous-jacent sans vous soucier que les appelants de ce code doivent être modifiés. La réduction du couplage entre les différents modules présente un réel avantage et vous devez absolument vous y efforcer.

Donc, si vous suivez les conseils que je viens de fournir, vous vous retrouverez avec quelque chose que vous avez déjà remarqué dans votre code, tout un tas de logique qui reste caché derrière une interface publique et n'est pas facilement accessible.

Idéalement, vous devriez pouvoir tester votre classe par rapport à son interface publique uniquement et si elle a des dépendances externes, vous devrez peut-être introduire des objets fake / mock / stub pour isoler le code testé.

Cependant, si vous faites cela et que vous sentez toujours que vous ne pouvez pas tester facilement chaque partie de la classe, il est très probable que votre classe en fasse trop. Dans l'esprit du principe SRP , vous pouvez suivre ce que Michael Feathers appelle "Sprout Class Pattern" et extraire une partie de votre classe d'origine dans une nouvelle classe.

Dans votre exemple, vous avez une classe d'importateur qui est également responsable de la lecture d'un fichier texte. L'une de vos options consiste à extraire un morceau de code de lecture de fichier dans une classe InputFileParser distincte. Maintenant, toutes ces fonctions privées deviennent publiques et donc facilement testables. En même temps, la classe de l'analyseur n'est pas quelque chose qui est visible pour les clients externes (marquez-la comme "interne", ne publiez pas de fichiers d'en-tête ou ne la publiez simplement pas dans le cadre de votre API) car ceux-ci continueront à utiliser l'original importateur dont l'interface restera courte et douce.

Au lieu de mettre beaucoup d'appels de méthode dans votre constructeur de classe - ce que je reconnais être une habitude à éviter en général - vous pouvez plutôt créer une méthode d'usine pour gérer tous les trucs d'initialisation supplémentaires que vous ne voulez pas déranger le client côté avec. Cela signifierait que vous exposeriez une méthode pour qu'elle ressemble à votre constructGroupOfCustomers()méthode en tant que public, et que vous l'utilisiez comme méthode statique de votre classe:

GroupOfCustomersImporter importer = 
    new GroupOfCustomersImporter.CreateInstance();

ou comme méthode d'une classe d'usine séparée peut-être:

ImporterFactory factory = new ImporterFactory();
GroupOfCustomersImporter importer = factory.CreateGroupOfCustomersImporter();

Ce ne sont que les deux premières options auxquelles j'ai pensé tout de suite.

Cela vaut également la peine de considérer que votre instinct essaie de vous dire quelque chose. Lorsque votre instinct vous dit que le code commence à sentir, il sent probablement, et il vaut mieux faire quelque chose avant qu'il ne pue! Bien sûr, en apparence, il n'y a rien de intrinsèquement mauvais avec votre code en soi, mais une nouvelle vérification rapide et un remaniement vous aideront à régler vos réflexions sur le problème afin que vous puissiez passer à d'autres choses en toute confiance sans vous inquiéter si vous construisez un château de cartes potentiel. Dans ce cas particulier, déléguer certaines des responsabilités du constructeur à - ou être appelé par - une usine garantit que vous pouvez exercer un plus grand contrôle sur l'instanciation de votre classe et de ses futurs descendants potentiels.

Ajout de ma propre réponse, car elle s'est avérée trop longue pour un commentaire.

Vous avez absolument raison que l'encapsulation et les tests sont assez contraires - si vous cachez presque tout, il est difficile de tester.

Vous pouvez cependant rendre l'exemple plus testable en donnant un flux plutôt qu'un nom de fichier - de cette façon, vous fournissez simplement un csv connu à partir de la mémoire, ou un fichier si vous le souhaitez. Cela rendra également votre classe plus robuste lorsque vous aurez besoin d'ajouter le support http;)

Regardez également comment utiliser lazy-init:

public class Csv
{
  private CustomerList list;

  public CustomerList get()
  {
    if(list == null)
        load();
     return list;
  }
}

Le constructeur ne doit pas faire grand-chose sauf l'initialisation de certaines variables ou l'état de l'objet. En faire trop dans le constructeur peut déclencher des exceptions d'exécution. J'essaierais d'éviter que le client fasse quelque chose comme ça:

MyClass a;
try {
   a = new MyClass();
} catch (MyException e) {
   //do something
}

Au lieu de faire:

MyClass a = new MyClass(); // Or a factory method
try {
   a.doSomething();
} catch (MyException e) {
   //do something
}