Uzasadnienie, aby preferować zmienne lokalne zamiast zmiennych instancji?

Baza kodów, nad którą pracuję, często używa zmiennych instancji do udostępniania danych między różnymi trywialnymi metodami. Pierwotny programista jest przekonany, że przestrzega najlepszych praktyk zawartych w książce Clean Code autorstwa Uncle Bob / Robert Martin: „Pierwszą zasadą funkcji jest to, że powinny być małe”. oraz „Idealna liczba argumentów dla funkcji wynosi zero (niladic). (...) Argumenty są trudne. Wymagają dużej mocy konceptualnej”.

Przykład:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  private byte[] encodedData;
  private EncryptionInfo encryptionInfo;
  private EncryptedObject payloadOfResponse;
  private URI destinationURI;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    getEncodedData(encryptedRequest);
    getEncryptionInfo();
    getDestinationURI();
    passRequestToServiceClient();

    return cryptoService.encryptResponse(payloadOfResponse);
  }

  private void getEncodedData(EncryptedRequest encryptedRequest) {
    encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private void getEncryptionInfo() {
    encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
  }

  private void getDestinationURI() {
    destinationURI = router.getDestination().getUri();
  }

  private void passRequestToServiceClient() {
    payloadOfResponse = serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }
}

Zmieniłbym to na następujące, używając zmiennych lokalnych:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    byte[] encodedData = cryptoService.decryptRequest(encryptedRequest, byte[].class);
    EncryptionInfo encryptionInfo = cryptoService.getEncryptionInfoForDefaultClient();
    URI destinationURI = router.getDestination().getUri();
    EncryptedObject payloadOfResponse = serviceClient.handle(destinationURI, encodedData,
      encryptionInfo);

    return cryptoService.encryptResponse(payloadOfResponse);
  }
}

Jest to krótsze, eliminuje niejawne sprzężenie danych między różnymi trywialnymi metodami i ogranicza zakresy zmiennych do wymaganego minimum. Jednak pomimo tych korzyści nadal nie wydaje mi się, aby przekonać pierwotnego programistę, że takie refaktoryzowanie jest uzasadnione, ponieważ wydaje się być sprzeczne z praktykami wuja Boba wspomnianymi powyżej.

Stąd moje pytania: jaki jest obiektywny, naukowy uzasadnienie faworyzowania zmiennych lokalnych nad zmiennymi instancji? Nie mogę po prostu położyć na tym palca. Moja intuicja mówi mi, że ukryte połączenia są złe i że wąski zakres jest lepszy niż szeroki. Ale jaka jest nauka, aby to poprzeć?

I odwrotnie, czy są jakieś wady tego refaktoryzacji, które prawdopodobnie przeoczyłem?

Jaki jest cel, naukowe uzasadnienie faworyzowania zmiennych lokalnych nad zmiennymi instancji?

Zakres nie jest stanem binarnym, jest gradientem. Możesz uszeregować je od największego do najmniejszego:

Global > Class > Local (method) > Local (code block, e.g. if, for, ...)

Edycja: to, co nazywam „zakresem klasy”, oznacza „zmienną instancji”. Według mojej wiedzy są one synonimami, ale jestem programistą C #, a nie programistą Java. Ze względu na zwięzłość umieściłem wszystkie statyki w globalnej kategorii, ponieważ statyka nie jest tematem pytania.

Im mniejszy zakres, tym lepiej. Uzasadnieniem jest to, że zmienne powinny żyć w możliwie najmniejszym zakresie . Jest na to wiele korzyści:

• Zmusza cię do myślenia o odpowiedzialności obecnej klasy i pomaga trzymać się SRP.
• Pozwala on nie musiał uniknąć globalne konflikty nazewnictwa, np jeśli dwie lub więcej klas posiada Namenieruchomość, nie jesteś zmuszony do poprzedzić je jak FooName, BarName... Zatem trzymając nazw zmiennych jako czyste i zwięzła, jak to możliwe.
• Odszyfrowuje kod, ograniczając dostępne zmienne (np. Intellisense) do tych, które są istotne kontekstowo.
• Umożliwia jakąś formę kontroli dostępu, dzięki czemu twoich danych nie może manipulować żaden nieznany aktor (np. Inna klasa opracowana przez kolegę).
• Dzięki temu kod jest bardziej czytelny, ponieważ zapewnia się, że deklaracja tych zmiennych stara się być jak najbardziej zbliżona do faktycznego wykorzystania tych zmiennych.
• Bezmyślne deklarowanie zmiennych w zbyt szerokim zakresie często wskazuje na programistę, który nie do końca rozumie OOP lub jak go wdrożyć. Widzenie zbyt szeroko zakrojonych zmiennych działa jak czerwona flaga, że ​​prawdopodobnie coś jest nie tak z podejściem OOP (albo ogólnie z deweloperem, albo z bazą kodu w szczególności).
• (Komentarz Kevin) Korzystanie z mieszkańców zmusza cię do robienia rzeczy we właściwej kolejności. W oryginalnym kodzie (zmiennej klasy) można niepoprawnie przejść passRequestToServiceClient()na początek metody i nadal się kompiluje. W przypadku miejscowych możesz popełnić ten błąd tylko wtedy, gdy przekażesz niezainicjowaną zmienną, co, mam nadzieję, jest wystarczająco oczywiste, że tak naprawdę tego nie robisz.

Jednak pomimo tych korzyści nadal nie wydaje mi się, aby przekonać pierwotnego programistę, że takie refaktoryzowanie jest uzasadnione, ponieważ wydaje się być sprzeczne z praktykami wuja Boba wspomnianymi powyżej.

I odwrotnie, czy są jakieś wady tego refaktoryzacji, które prawdopodobnie przeoczyłem?

Problem polega na tym, że Twój argument dotyczący zmiennych lokalnych jest prawidłowy, ale wprowadziłeś również dodatkowe zmiany, które nie są poprawne i powodują, że sugerowana poprawka nie powiedzie się testowi zapachu.

Chociaż rozumiem twoją sugestię „bez zmiennej klasowej” i jest to uzasadniona, w rzeczywistości również usunąłeś same metody, i to jest zupełnie inna gra. Metody powinny pozostać, a zamiast tego powinieneś je zmienić, aby zwracały swoją wartość, zamiast przechowywać ją w zmiennej klasy:

private byte[] getEncodedData() {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
}

private EncryptionInfo getEncryptionInfo() {
    return cryptoService.getEncryptionInfoForDefaultClient();
}

// and so on...

Zgadzam się z tym, co zrobiłeś w processmetodzie, ale powinieneś był wywoływać prywatne metody, a nie wykonywać ich ciała bezpośrednio.

public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    byte[] encodedData = getEncodedData();
    EncryptionInfo encryptionInfo = getEncryptionInfo();

    //and so on...

    return cryptoService.encryptResponse(payloadOfResponse);
}

Potrzebujesz dodatkowej warstwy abstrakcji, zwłaszcza gdy natrafisz na metody, które muszą być ponownie użyte kilka razy. Nawet jeśli obecnie nie używasz ponownie swoich metod , dobrą praktyką jest już tworzenie podmodeli tam, gdzie to stosowne, nawet jeśli tylko w celu zwiększenia czytelności kodu.

Niezależnie od argumentu zmiennej lokalnej od razu zauważyłem, że sugerowana poprawka jest znacznie mniej czytelna niż oryginał. Przyznaję, że bezmyślne użycie zmiennych klas również szkodzi czytelności kodu, ale nie na pierwszy rzut oka w porównaniu z tym, że ułożyłeś całą logikę w jedną (teraz długotrwałą) metodę.

Oryginalny kod używa zmiennych składowych, takich jak argumenty. Kiedy mówi, aby zminimalizować liczbę argumentów, tak naprawdę ma na myśli minimalizację ilości danych wymaganych przez metody do działania. Umieszczenie tych danych w zmiennych członka nic nie poprawi.

Inne odpowiedzi już doskonale wyjaśniły zalety zmiennych lokalnych, więc pozostaje tylko ta część twojego pytania:

Jednak pomimo tych korzyści nadal nie wydaje mi się, aby przekonać pierwotnego programistę, że takie refaktoryzowanie jest uzasadnione, ponieważ wydaje się być sprzeczne z praktykami wuja Boba wspomnianymi powyżej.

To powinno być łatwe. Po prostu skieruj go na następujący cytat w Czystym kodzie wuja Boba:

Nie mają skutków ubocznych

Skutki uboczne to kłamstwa. Twoja funkcja obiecuje zrobić jedną rzecz, ale robi też inne ukryte rzeczy. Czasami wprowadzi nieoczekiwane zmiany do zmiennych własnej klasy. Czasami doprowadzi to do parametrów przekazanych do funkcji lub globałów systemowych. W obu przypadkach są przebiegłymi i niszczącymi niewiernościami, które często powodują dziwne połączenia doczesne i zależności od porządku.

(przykład pominięty)

Ten efekt uboczny tworzy połączenie czasowe. Oznacza to, że checkPassword można wywoływać tylko w określonych momentach (innymi słowy, kiedy można bezpiecznie zainicjować sesję). Jeśli zostanie wywołany poza kolejnością, dane sesji mogą zostać przypadkowo utracone. Złącza czasowe są mylące, szczególnie gdy są ukryte jako efekt uboczny. Jeśli musisz mieć połączenie czasowe, powinieneś to wyraźnie zaznaczyć w nazwie funkcji. W takim przypadku możemy zmienić nazwę funkcji checkPasswordAndInitializeSession, choć z pewnością narusza to „Zrób jedną rzecz”.

Oznacza to, że wujek Bob nie tylko mówi, że funkcja powinna przyjmować kilka argumentów, ale także mówi, że funkcje powinny unikać interakcji ze stanem nielokalnym, gdy tylko jest to możliwe.

„Zaprzecza temu, co myśli wujek”, NIGDY nie jest dobrym argumentem. NIGDY. Nie bierz mądrości od wujków, pomyśl sam.

To powiedziawszy, zmienne instancji powinny być używane do przechowywania informacji, które faktycznie muszą być przechowywane trwale lub półtrwale. Informacje tutaj nie są. Bardzo łatwo jest żyć bez zmiennych instancji, więc mogą odejść.

Test: Napisz komentarz do dokumentacji dla każdej zmiennej instancji. Czy potrafisz napisać coś, co nie jest całkowicie bezcelowe? I napisz komentarz do dokumentacji do czterech akcesorów. Są równie bezcelowe.

Najgorsze jest założenie, że odszyfrujesz zmiany, ponieważ używasz innej usługi cryptoService. Zamiast zmieniać cztery wiersze kodu, musisz zastąpić cztery zmienne instancji różnymi, cztery pobierające różne i zmienić cztery wiersze kodu.

Ale oczywiście pierwsza wersja jest lepsza, jeśli płacisz według linii kodu. 31 linii zamiast 11 linii. Trzy razy więcej wierszy do napisania i do zachowania na zawsze, do czytania, gdy coś debugujesz, do dostosowywania, gdy potrzebne są zmiany, do powielania, jeśli wspierasz drugą usługę cryptoService.

(Pominięto ważny punkt, że użycie zmiennych lokalnych zmusza do wykonywania połączeń w odpowiedniej kolejności).

Jaki jest cel, naukowe uzasadnienie faworyzowania zmiennych lokalnych nad zmiennymi instancji? Nie mogę po prostu położyć na tym palca. Moja intuicja mówi mi, że ukryte połączenia są złe i że wąski zakres jest lepszy niż szeroki. Ale jaka jest nauka, aby to poprzeć?

Zmienne instancji służą do reprezentowania właściwości ich obiektu nadrzędnego, a nie do reprezentowania właściwości specyficznych dla wątków obliczeń węższych niż sam obiekt. Niektóre z powodów wprowadzenia takiego rozróżnienia, które, jak się wydaje, jeszcze nie zostały uwzględnione, dotyczą współbieżności i ponownego ustalenia. Jeśli metody wymieniają dane, ustawiając wartości zmiennych instancji, wówczas dwa współbieżne wątki mogą z łatwością wzajemnie spychać wartości tych zmiennych instancji, co powoduje sporadyczne, trudne do znalezienia błędy.

Nawet jeden wątek może napotykać problemy wzdłuż tych linii, ponieważ istnieje wysokie ryzyko, że wzorzec wymiany danych oparty na zmiennych instancji spowoduje, że metody nie będą ponownie stosowane. Podobnie, jeśli te same zmienne są używane do przesyłania danych między różnymi parami metod, istnieje ryzyko, że pojedynczy wątek wykonujący nawet nierekurencyjny łańcuch wywołań metod natknie się na błędy obracające się wokół nieoczekiwanych modyfikacji zaangażowanych zmiennych instancji.

Aby uzyskać wiarygodne prawidłowe wyniki w takim scenariuszu, musisz albo użyć osobnych zmiennych do komunikacji między każdą parą metod, w których jedna wywołuje drugą, albo też aby każda implementacja metody uwzględniała wszystkie szczegóły implementacji pozostałych metody, które wywołuje, bezpośrednio lub pośrednio. Jest kruchy i źle się skaluje.

Tylko omawiając process(...)przykład twoich kolegów jest znacznie bardziej czytelny w sensie logiki biznesowej. I odwrotnie, twój przeciwny przykład wymaga więcej niż pobieżnego spojrzenia, aby wydobyć jakiekolwiek znaczenie.

To powiedziawszy, czysty kod jest zarówno czytelny, jak i dobrej jakości - wypychanie lokalnego stanu do bardziej globalnej przestrzeni to po prostu montaż na wysokim poziomie, więc zero dla jakości.

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest request) {
    checkNotNull(encryptedRequest);

    return encryptResponse
      (routeTo
         ( destination()
         , requestData(request)
         , destinationEncryption()
         )
      );
  }

  private byte[] requestData(EncryptedRequest encryptedRequest) {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private EncryptionInfo destinationEncryption() {
    return cryptoService.getEncryptionInfoForDefaultClient();
  }

  private URI destination() {
    return router.getDestination().getUri();
  }

  private EncryptedObject routeTo(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {
    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }

  private void encryptResponse(EncryptedObject payloadOfResponse) {
    return cryptoService.encryptResponse(payloadOfResponse);
  }
}

Jest to wersja, która eliminuje potrzebę zmiennych w dowolnym zakresie. Tak, kompilator je wygeneruje, ale ważną częścią jest to, że kontroluje to, aby kod był wydajny. Jest również stosunkowo czytelny.

Tylko kwestia nazewnictwa. Chcesz mieć najkrótszą nazwę, która jest znacząca i rozwija się na podstawie już dostępnych informacji. to znaczy. destinationURI, „URI” jest już znany z podpisu typu.

Chciałbym po prostu całkowicie usunąć te zmienne i metody prywatne. Oto mój refaktor:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    return cryptoService.encryptResponse(
        serviceClient.handle(router.getDestination().getUri(),
        cryptoService.decryptRequest(encryptedRequest, byte[].class),
        cryptoService.getEncryptionInfoForDefaultClient()));
  }
}

W przypadku metody prywatnej, np. router.getDestination().getUri()Jest jaśniejszy i bardziej czytelny niż getDestinationURI(). Chciałbym nawet powtórzyć, że jeśli użyję tej samej linii dwa razy w tej samej klasie. Aby spojrzeć na to z innej strony, jeśli istnieje potrzeba getDestinationURI(), prawdopodobnie należy do innej klasy, a nie do SomeBusinessProcessklasy.

W przypadku zmiennych i właściwości powszechną ich potrzebą jest przechowywanie wartości, które zostaną później wykorzystane. Jeśli klasa nie ma publicznego interfejsu dla właściwości, prawdopodobnie nie powinny to być właściwości. Najgorszym rodzajem wykorzystywanych właściwości klas jest prawdopodobnie przekazywanie wartości między metodami prywatnymi za pomocą efektów ubocznych.

W każdym razie klasa musi tylko zrobić, process()a następnie obiekt zostanie wyrzucony, nie ma potrzeby utrzymywania żadnego stanu w pamięci. Dalszym potencjałem refaktora byłoby usunięcie CryptoService z tej klasy.

Na podstawie komentarzy chcę dodać, że ta odpowiedź jest oparta na praktyce w świecie rzeczywistym. Rzeczywiście, podczas przeglądu kodu, pierwszą rzeczą, którą wybrałem, jest refaktoryzacja klasy i przeniesienie pracy szyfrowania / deszyfrowania. Gdy to zrobisz, zapytam, czy metody i zmienne są potrzebne, czy są poprawnie nazwane i tak dalej. Ostateczny kod prawdopodobnie będzie bliżej tego:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;

  public Response process(Request request) {
    return serviceClient.handle(router.getDestination().getUri());
  }
}

W przypadku powyższego kodu nie sądzę, że wymaga on dalszego refaktoryzacji. Podobnie jak w przypadku zasad, myślę, że potrzeba doświadczenia, aby wiedzieć, kiedy i kiedy ich nie stosować. Reguły nie są teoriami, które sprawdziły się we wszystkich sytuacjach.

Z drugiej strony, przegląd kodu ma realny wpływ na to, ile czasu upłynie, zanim fragment kodu może przejść. Moją sztuczką jest mieć mniej kodu i ułatwić zrozumienie. Nazwa zmiennej może być punktem dyskusji, jeśli mogę ją usunąć, recenzenci nawet nie będą musieli o tym myśleć.

Odpowiedź Flatera całkiem dobrze obejmuje kwestie określania zakresu, ale myślę, że jest tu także inny problem.

Zauważ, że istnieje różnica między funkcją przetwarzającą dane a funkcją, która po prostu uzyskuje dostęp do danych .

Pierwszy z nich realizuje rzeczywistą logikę biznesową, podczas gdy drugi oszczędza pisania i być może zwiększa bezpieczeństwo, dodając prostszy i łatwiejszy w użyciu interfejs.

W tym przypadku wydaje się, że funkcje dostępu do danych nie zapisują pisania i nie są nigdzie ponownie używane (lub byłyby inne problemy z ich usunięciem). Te funkcje po prostu nie powinny istnieć.

Zachowując tylko logikę biznesową w nazwanych funkcjach, uzyskujemy to, co najlepsze z obu światów (gdzieś pomiędzy odpowiedzią Flatera a odpowiedzią imel96 ):

public EncryptedResponse process(EncryptedRequest encryptedRequest) {

    byte[] requestData = decryptRequest(encryptedRequest);
    EncryptedObject responseData = handleRequest(router.getDestination().getUri(), requestData, cryptoService.getEncryptionInfoForDefaultClient());
    EncryptedResponse response = encryptResponse(responseData);

    return response;
}

// define: decryptRequest(), handleRequest(), encryptResponse()

Pierwsza i najważniejsza rzecz: wujek Bob czasami wydaje się być kaznodzieją, ale stwierdza, że ​​istnieją wyjątki od jego zasad.

Cała idea Clean Code polega na poprawie czytelności i uniknięciu błędów. Istnieje kilka zasad, które wzajemnie się naruszają.

Argumentuje on za funkcjami, że funkcje niladyczne są najlepsze, jednak dopuszczalne są maksymalnie trzy parametry. Osobiście uważam, że 4 są również w porządku.

Kiedy używane są zmienne instancji, powinny one tworzyć spójną klasę. Oznacza to, że zmienne powinny być stosowane w wielu, jeśli nie we wszystkich metodach niestatycznych.

Zmienne, które nie są używane w wielu miejscach klasy, powinny zostać przeniesione.

Nie uważałbym, że wersja oryginalna ani wersja refaktoryzowana są optymalne, a @Flater już bardzo dobrze stwierdził, co można zrobić z wartościami zwracanymi. Poprawia to czytelność i zmniejsza liczbę błędów, aby użyć zwracanych wartości.

Zmienne lokalne zmniejszają zakres, a zatem ograniczają sposoby wykorzystania zmiennych, a tym samym pomagają zapobiegać pewnym klasom błędów i poprawiają czytelność.

Zmienna instancji zmniejsza sposoby wywoływania funkcji, co pomaga również ograniczyć niektóre klasy błędów i poprawia czytelność.

Stwierdzenie, że jedno ma rację, a drugie nie, może być słusznym wnioskiem w każdym konkretnym przypadku, ale jako ogólna rada ...

TL; DR: Myślę, że powodem, dla którego czujesz zbyt dużo zapału, jest zbyt dużo zapału.

Pomimo faktu, że metody zaczynające się od get ... nie powinny powrócić do pustki, oddzielenie poziomów abstrakcji w ramach metod podano w pierwszym rozwiązaniu. Chociaż drugie rozwiązanie ma większy zakres, nadal trudniej jest zrozumieć, co dzieje się w metodzie. Przypisania zmiennych lokalnych nie są tutaj potrzebne. Zachowałbym nazwy metod i zmieniłem kod na coś takiego:

public class SomeBusinessProcess {
  @Inject private Router router;
  @Inject private ServiceClient serviceClient;
  @Inject private CryptoService cryptoService;

  public EncryptedResponse process(EncryptedRequest encryptedRequest) {
    checkNotNull(encryptedRequest);

    return getEncryptedResponse(
            passRequestToServiceClient(getDestinationURI(), getEncodedData(encryptedRequest) getEncryptionInfo())
        );
  }

  private EncryptedResponse getEncryptedResponse(EncryptedObject encryptedObject) {
    return cryptoService.encryptResponse(encryptedObject);
  }

  private byte[] getEncodedData(EncryptedRequest encryptedRequest) {
    return cryptoService.decryptRequest(encryptedRequest, byte[].class);
  }

  private EncryptionInfo getEncryptionInfo() {
    return cryptoService.getEncryptionInfoForDefaultClient();
  }

  private URI getDestinationURI() {
    return router.getDestination().getUri();
  }

  private EncryptedObject passRequestToServiceClient(URI destinationURI, byte[] encodedData, EncryptionInfo encryptionInfo) {
    return serviceClient.handle(destinationURI, encodedData, encryptionInfo);
  }
}

Obie rzeczy robią to samo, a różnice w wydajności są niezauważalne, więc nie sądzę, aby istniał argument naukowy . Sprowadza się to wtedy do subiektywnych preferencji.

Ja też lubię twoją drogę bardziej niż twoją koleżankę. Dlaczego? Ponieważ myślę, że łatwiej jest czytać i rozumieć, pomimo tego, co mówi autor niektórych książek.

Oba sposoby osiągają to samo, ale jego droga jest bardziej rozproszona. Aby odczytać ten kod, musisz przełączać się między kilkoma funkcjami i zmiennymi składowymi. Nie wszystko jest skondensowane w jednym miejscu, musisz pamiętać o tym wszystkim w swojej głowie, aby to zrozumieć. To znacznie większy ładunek poznawczy.

W przeciwieństwie do tego, twoje podejście pakuje wszystko o wiele bardziej gęsto, ale nie tak, aby uczynić je nieprzeniknionym. Po prostu czytasz to wiersz po wierszu i nie musisz tak dużo zapamiętywać, aby to zrozumieć.

Jeśli jednak przyzwyczaił się do takiego układania kodu, mogę sobie wyobrazić, że dla niego może być odwrotnie.