Czy cierpię z powodu nadużywania enkapsulacji?

Zauważyłem coś w moim kodzie w różnych projektach, które wydaje mi się, że kod pachnie i coś złego do zrobienia, ale nie mogę sobie z tym poradzić.

Próbując pisać „czysty kod”, często nadużywam prywatnych metod, aby mój kod był łatwiejszy do odczytania. Problem polega na tym, że kod jest rzeczywiście czystszy, ale trudniej go przetestować (tak, wiem, że mogę przetestować metody prywatne ...) i ogólnie wydaje mi się to złym nawykiem.

Oto przykład klasy, która odczytuje niektóre dane z pliku .csv i zwraca grupę klientów (inny obiekt z różnymi polami i atrybutami).

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

Jak widać, klasa ma tylko konstruktor, który wywołuje niektóre prywatne metody wykonania zadania, wiem, że nie jest to ogólnie dobra praktyka, ale wolę obudować całą funkcjonalność w klasie, zamiast upubliczniać metody. klient powinien działać w ten sposób:

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

Wolę to, ponieważ nie chcę umieszczać niepotrzebnych wierszy kodu w kodzie bocznym klienta, przeszkadzając klasie klienta szczegółami implementacji.

Czy to rzeczywiście zły nawyk? Jeśli tak, jak mogę tego uniknąć? Pamiętaj, że powyższy jest tylko prostym przykładem. Wyobraź sobie, że ta sama sytuacja dzieje się w czymś nieco bardziej złożonym.

Myślę, że jesteś na dobrej drodze, jeśli chcesz ukryć szczegóły implementacji przed klientem. Chcesz zaprojektować swoją klasę, aby interfejs, który widzi klient, był najprostszym i najbardziej zwięzłym interfejsem API, jaki możesz wymyślić. Klient nie tylko nie będzie „przeszkadzał” w szczegółach implementacji, ale także będzie mógł bez problemu refaktoryzować kod źródłowy, nie martwiąc się, że wywołujący go kod musi zostać zmodyfikowany. Zmniejszenie sprzężenia między różnymi modułami przynosi pewne realne korzyści i zdecydowanie powinieneś do tego dążyć.

Więc jeśli zastosujesz się do podanej wcześniej porady, skończysz z czymś, co już zauważyłeś w kodzie, całą logiką, która pozostaje ukryta za publicznym interfejsem i nie jest łatwo dostępna.

Idealnie, powinieneś być w stanie przetestować klasę tylko na interfejsie publicznym, a jeśli ma zewnętrzne zależności, może być konieczne wprowadzenie fałszywych / próbnych / skrótowych obiektów w celu wyizolowania testowanego kodu.

Jeśli jednak to zrobisz i nadal czujesz, że nie możesz łatwo przetestować każdej części klasy, bardzo prawdopodobne jest, że Twoja jedna klasa robi zbyt wiele. W duchu zasady SRP możesz postępować zgodnie z tym, co Michael Feathers nazywa „Wzorzec klasy kiełków” i wyodrębnić fragment swojej oryginalnej klasy do nowej klasy.

W twoim przykładzie masz klasę importerów, która jest również odpowiedzialna za odczyt pliku tekstowego. Jedną z opcji jest wyodrębnienie fragmentu kodu do odczytu pliku do osobnej klasy InputFileParser. Teraz wszystkie te prywatne funkcje stają się publiczne i dlatego można je łatwo przetestować. Jednocześnie klasa parsera nie jest czymś, co jest widoczne dla klientów zewnętrznych (oznacz ją jako „wewnętrzną”, nie publikuj plików nagłówków lub po prostu nie reklamuj jej jako części interfejsu API), ponieważ będą one nadal używać oryginalnej importer, którego interfejs pozostanie krótki i słodki.

Alternatywą dla umieszczania wielu wywołań metod w konstruktorze klas - co, jak się zgadzam, jest zwyczajowo unikać - można zamiast tego stworzyć metodę fabryczną do obsługi wszystkich dodatkowych elementów inicjujących, których nie chcesz niepokoić klientowi bok z. Oznaczałoby to, że ujawniasz metodę, która wygląda jak constructGroupOfCustomers()metoda publiczna, i używasz jej jako statycznej metody klasy:

GroupOfCustomersImporter importer = 
    new GroupOfCustomersImporter.CreateInstance();

lub jako metoda oddzielnej klasy fabrycznej, być może:

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

To tylko kilka pierwszych opcji, które wymyśliłem od razu na czubku głowy.

Warto również wziąć pod uwagę, że instynkty próbują ci coś powiedzieć. Kiedy jelito mówi ci, że kod zaczyna wąchać, prawdopodobnie pachnie i lepiej coś z tym zrobić, zanim cuchnie! Pewnie na pierwszy rzut oka może nie być z samym kodem nic złego, jednak szybka weryfikacja i refaktor pomogą rozwiązać twoje przemyślenia na temat problemu, dzięki czemu możesz śmiało przejść do innych rzeczy bez obaw, jeśli budujesz potencjalny domek z kart. W tym konkretnym przypadku przekazanie części obowiązków konstruktora do - lub być wezwanym przez - fabrykę zapewnia, że ​​możesz sprawować większą kontrolę nad tworzeniem instancji swojej klasy i jej potencjalnych przyszłych potomków.

Dodanie własnej odpowiedzi, ponieważ skończyło się to zbyt długo na komentarz.

Masz całkowitą rację, że enkapsulacja i testowanie są zupełnie przeciwne - jeśli ukryjesz prawie wszystko, trudno jest to przetestować.

Możesz jednak uczynić ten przykład bardziej testowalnym, podając strumień zamiast nazwy pliku - w ten sposób wystarczy podać znany plik csv z pamięci lub plik, jeśli chcesz. Dzięki temu Twoja klasa będzie bardziej niezawodna, gdy będziesz musiał dodać obsługę HTTP;)

Zobacz także lazy-init:

public class Csv
{
  private CustomerList list;

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

Konstruktor nie powinien robić zbyt wiele poza inicjowaniem niektórych zmiennych lub stanu obiektu. Wykonanie zbyt dużej czynności w konstruktorze może spowodować wyjątki w czasie wykonywania. Starałbym się unikać wykonywania przez klienta czegoś takiego:

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

Zamiast robić:

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