Nazwa tego antipattern? Pola jako zmienne lokalne [zamknięte]

68

W niektórych kodach, które przeglądam, widzę rzeczy, które są moralnym odpowiednikiem:

public class Foo
{
    private Bar bar;

    public MethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public MethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

To pole barjest logicznie zmienną lokalną, ponieważ jego wartość nigdy nie ma trwać w wywołaniach metod. Ponieważ jednak wiele metod Foowymaga obiektu typu Bar, autor oryginalnego kodu właśnie utworzył pole typu Bar.

  1. To jest oczywiście złe, prawda?
  2. Czy istnieje nazwa tego antypatternu?
JSB ձոգչ
źródło
60
Cóż, to jest nowe. Mam nadzieję, że ta klasa nie jest używana w kontekście wielowątkowym, w przeciwnym razie będziesz miał ciekawe czasy!
TMN
8
To jest naprawdę rzadkie? Widziałem to wiele razy w mojej karierze.
JSB
32
@TMN To nie jest bezpieczne dla wątków, ale co gorsza, nie jest to nawet ponowne wysłanie. Jeśli dowolny kod w MethodAlub MethodBprzyczyna MethodAlub MethodBzostać wywołana w jakikolwiek sposób (i barjest wykorzystywana ponownie, w przypadku bar.{A,B}()bycia sprawcy) masz podobne problemy nawet bez współbieżności.
73
Czy musimy mieć imię dla każdej głupiej rzeczy, którą ktoś mógłby zrobić? Po prostu nazwij to złym pomysłem.
Caleb
74
Trudno nie nazwać tego zwisającym prywatnym anty-wzorem.
psr

Odpowiedzi:

86

To jest oczywiście złe, prawda?

Tak.

  • To sprawia, że ​​metody nie są ponownie wysyłane, co stanowi problem, jeśli są one wywoływane w tej samej instancji rekurencyjnie lub w kontekście wielowątkowym.

  • Oznacza to, że stan z jednego połączenia wycieka do drugiego (jeśli zapomnisz ponownie zainicjować).

  • Utrudnia to zrozumienie kodu, ponieważ musisz sprawdzić powyższe, aby upewnić się, co faktycznie robi kod. (Porównaj z użyciem zmiennych lokalnych).

  • Sprawia, że ​​każda Fooinstancja jest większa niż powinna. (I wyobraź sobie, że robisz to dla N zmiennych ...)

Czy istnieje nazwa tego antypatternu?

IMO nie zasługuje na miano antypatternu. To tylko zły nawyk kodowania / niewłaściwe użycie konstrukcji Java / kod bzdury. Jest to coś, co można zobaczyć w kodzie licencjackim, gdy uczeń pomija wykłady i / lub nie ma umiejętności programistycznych. Jeśli widzisz to w kodzie produkcyjnym, jest to znak, że musisz zrobić o wiele więcej recenzji kodu ...


Dla przypomnienia poprawnym sposobem napisania tego jest:

public class Foo
{
    public methodA()
    {
        Bar bar = new Bar();  // Use a >>local<< variable!!
        bar.a();
    }

    // Or more concisely (in this case) ...
    public methodB()
    {
        new Bar().b();
    }
}

Zauważ, że poprawiłem także nazwy metod i zmiennych, aby były zgodne z przyjętymi regułami stylu dla identyfikatorów Java.

Stephen C.
źródło
11
Powiedziałbym: „Jeśli widzisz to w kodzie produkcyjnym, to znak, że powinieneś ponownie przeanalizować swój proces rekrutacji
Beta
@Beta - to też, chociaż powinieneś być w stanie skorygować takie złe nawyki kodowania dzięki energicznemu sprawdzaniu kodu.
Stephen C
+1 za nieco więcej recenzji kodu. Pomimo tego, co ludzie myślą, poprawa praktyki zatrudniania nie zapobiegnie tego rodzaju problemom - zatrudnianie to bzdura, najlepiej co możesz zrobić, to załadować kostki na swoją korzyść.
mattnz
@StephenC „Sprawia, że ​​metody nie są ponownie wysyłane, co stanowi problem, jeśli są wywoływane w tej samej instancji rekurencyjnie lub w kontekście wielowątkowym.” . Wiem, co to jest ponowne odniesienie, ale nie widzę tutaj sensu, ponieważ metody nie używają żadnych blokad? Czy możesz wyjaśnić, proszę.
Geek
@Geek - Za bardzo koncentrujesz się na konkretnym przykładzie. Mówię o ogólnym przypadku, w którym metoda może zostać wywołana z wielu wątków lub rekurencyjnie.
Stephen C
39

Nazwałbym to niepotrzebnym dużym zakresem dla zmiennej. To ustawienie pozwala również na warunki wyścigu, jeśli wiele wątków ma dostęp do metody A i metody B.

00 m
źródło
12
W szczególności myślę, że „zmienny zakres” to tutaj nazwa problemu. Ta osoba musi dowiedzieć się, jakie są zmienne lokalne i jak / kiedy ich używać. Wzorzec pozytywny lub ogólna zasada polega na użyciu najmniejszego zakresu dostępnego dla każdej zmiennej i rozszerzaniu go tylko w razie potrzeby. Żeby było jasne, ten błąd to nie tylko zły styl. Kodowanie takiego wyścigu jest błędem.
GlenPeterson
30

Jest to szczególny przypadek ogólnego wzorca znanego jako global doorknobbing. Budując dom, kusi, aby kupić tylko jedną klamkę i pozostawić ją leżącą. Kiedy ktoś chce użyć drzwi lub szafki, po prostu chwyta tę jedną globalną klamkę. Jeśli klamki są drogie, może to być dobry projekt.

Niestety, kiedy twój przyjaciel podchodzi i klamka jest jednocześnie używana w dwóch różnych miejscach, rzeczywistość się rozpada. Jeśli klamka jest tak droga, że ​​możesz sobie na nią pozwolić, warto zbudować system, który bezpiecznie pozwoli ludziom czekać na swoją kolej.

W tym przypadku klamka jest tylko odniesieniem, więc jest tania. Jeśli klamka była rzeczywistym obiektem (a oni ponownie używali tego obiektu zamiast tylko odniesienia), to może być wystarczająco kosztowne, aby być prudent global doorknob. Jednak gdy jest tani, jest znany jako cheapskate global doorknob.

Twój przykład jest cheapskateróżnorodny.

Ned Twigg
źródło
19

Podstawową kwestią jest współbieżność - jeśli Foo jest używany przez wiele wątków, masz problem.

Poza tym jest to po prostu głupie - lokalne zmienne doskonale zarządzają swoimi cyklami życia. Zastąpienie ich zmiennymi instancji, które należy zerować, gdy nie są już przydatne, jest zaproszeniem do błędu.

Matthew Flynn
źródło
15

Jest to szczególny przypadek „niewłaściwego określania zakresu” ze stroną „zmiennego ponownego wykorzystania”.

ikegami
źródło
7

To nie jest anty-wzór. Anty-wzory mają pewną właściwość, która sprawia, że ​​wydaje się to dobrym pomysłem, co prowadzi ludzi do celowego działania; są planowane jako wzory, a potem idzie to bardzo źle.

To także sprawia, że ​​debaty na temat tego, czy coś jest wzorem, anty-wzorem lub często źle stosowanym wzorem, który nadal ma zastosowanie w niektórych miejscach.

To jest po prostu złe.

Aby dodać trochę więcej.

Ten kod jest przesądny lub, w najlepszym razie, praktyką kultu ładunków.

Przesąd jest czymś zrobionym bez wyraźnego uzasadnienia. Może to być związane z czymś realnym, ale połączenie nie jest logiczne.

Praktyka kultu ładunku polega na tym, że próbujesz skopiować coś, czego nauczyłeś się z bardziej kompetentnego źródła, ale w rzeczywistości kopiujesz artefakty powierzchniowe, a nie proces (tak nazwany od kultu w Papui Nowej Gwinei, który stworzyłby bambusowe radia kontrolne z nadzieją na powrót japońskich i amerykańskich samolotów z II wojny światowej).

W obu tych przypadkach nie ma żadnego prawdziwego przypadku.

Anty-wzorzec jest próbą rozsądnej poprawy, czy to w małym (tym dodatkowym rozgałęzieniu, aby poradzić sobie z tym dodatkowym przypadkiem, z którym trzeba się uporać, co prowadzi do kodu spaghetti), czy w dużym, w którym bardzo celowo wdrażasz wzorzec które są dyskredytowane lub dyskutowane (wiele osób opisałoby singletony jako takie, z których niektóre wykluczają tylko do zapisu - np. obiekty rejestrujące lub tylko do odczytu, np. obiekty ustawień konfiguracyjnych - a niektóre potępiłyby nawet te), albo też gdzie rozwiązujesz niewłaściwy problem (kiedy po raz pierwszy wprowadzono platformę .NET, MS zalecił wzorzec postępowania ze zutylizowaniem, gdy masz zarówno niezarządzane pola, jak i jednorazowe pola zarządzane - rzeczywiście radzi sobie z tą sytuacją bardzo dobrze, ale prawdziwy problem polega na tym, że masz oba typy pól w tej samej klasie).

Jako taki, anty-wzorzec jest czymś, co inteligentna osoba, która dobrze zna język, problematyczną domenę i dostępne biblioteki, celowo zrobi, co wciąż ma (lub twierdzi się, że ma) wadę, która przytłacza plusy.

Ponieważ nikt z nas nie zaczyna dobrze znać danego języka, problematycznej domeny i dostępnych bibliotek, a ponieważ każdy może coś przeoczyć, przechodząc od jednego rozsądnego rozwiązania do drugiego (np. Zacznij przechowywać coś w polu dla dobrego użytku, a następnie spróbuj przerób to, ale nie dokończ zadania, a skończysz z kodem jak w pytaniu), a ponieważ wszyscy od czasu do czasu czegoś brakuje w nauce, wszyscy stworzyliśmy jakiś przesądny lub kultowy kod w pewnym momencie . Dobrą rzeczą jest to, że w rzeczywistości są one łatwiejsze do zidentyfikowania i poprawienia niż anty-wzory. Prawdziwe anty-wzory albo nie są prawdopodobnie anty-wzorami, albo mają jakąś atrakcyjną jakość, albo przynajmniej mają jakiś sposób na zwabienie jednego z nich, nawet gdy zostaną zidentyfikowane jako złe (zbyt wiele i zbyt mało warstw jest obuwie kontrowersyjnych, ale unikanie jednego prowadzi do drugiego).

Jon Hanna
źródło
1
Ale ludzie nie uważają, że jest to dobry pomysł, prawdopodobnie dlatego, że myślę, że jest to forma kodu ponownego użycia.
JSB ձոգչ
Początkujący często myślą, że zadeklarowanie dwóch zmiennych w jakiś sposób wymaga podwójnej przestrzeni, dlatego wolą ponownie używać zmiennych. To pokazuje nieporozumienie modelu pamięci (darmowy magazyn vs. stos, możliwość ponownego wykorzystania lokalizacji stosu) i optymalizacje, które wszystkie kompilatory są w stanie wykonać. Twierdziłbym, że początkujący może zatem uznać ponowne użycie zmiennych za dobry pomysł, aby można go było zaklasyfikować jako antypattern.
Filip
To sprawia, że ​​jest to przesąd, a nie antypattern.
Jon Hanna,
3

(1) Powiedziałbym, że to nie jest dobre.

Wykonuje ręczne utrzymywanie domu, że stos może automatycznie robić ze zmiennymi lokalnymi.

Nie ma korzyści w zakresie wydajności, ponieważ „nowe” nazywane jest każdym wywołaniem metody.

EDYCJA: Ślad pamięci klasy jest większy, ponieważ wskaźnik słupkowy zawsze zajmuje pamięć przez cały czas istnienia klasy. Jeśli wskaźnik słupkowy byłby lokalny, używałby pamięci tylko na czas trwania wywołania metody. Pamięć wskaźnika to po prostu kropla w oceanie, ale wciąż jest to niepotrzebna kropla.

Pasek jest widoczny dla innych metod w klasie. Metody, które nie używają paska, nie powinny go widzieć.

Błędy oparte na stanie. W tym konkretnym przypadku błędy bazowe stanu powinny występować tylko w wielowątkowości, ponieważ za każdym razem wywoływane jest „nowe”.

(2) Nie wiem, czy jest na to jakaś nazwa, ponieważ w rzeczywistości jest to kilka problemów, a nie jeden.
- ręczne sprzątanie, gdy dostępna jest
funkcja automatyczna - stan konieczny - stan,
który żyje dłużej niż jego żywotność -
widoczność lub zakres jest zbyt wysoki

mike30
źródło
2

Nazwałbym to „Wędrującym Ksenofobem”. Chce zostać sam, ale kończy się nie wiedząc dokładnie, gdzie i co to jest. Dlatego jest to zła rzecz, jak stwierdzili inni.

Inżynier świata
źródło
2

Nie zgadzam się tutaj, ale chociaż nie uważam, że podany przykład jest dobrym stylem kodowania, ponieważ jest w obecnej formie, wzorzec istnieje podczas przeprowadzania testów jednostkowych.

Ten dość standardowy sposób przeprowadzania testów jednostkowych

public class BarTester
{
    private Bar bar;

    public Setup() { bar = new Bar(); }
    public Teardown() { bar = null; }

    public TestMethodA()
    {
        bar.A();        
    }

    public TestMethodB()
    {
        bar.B();
    }
}

jest tylko refaktoryzacją tego odpowiednika kodu OP

public class BarTester
{
    private Bar bar;

    public TestMethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public TestMethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

Nigdy nie napisałbym kodu podanego w przykładzie OP, krzyknął on o refaktoryzacji, ale uważam, że wzorzec nie jest ani nieprawidłowy, ani antypatograficzny .

Lieven Keersmaekers
źródło
W takim przypadku urządzenie jest tworzone niezależnie dla każdego testu, a problemy związane z ponownym użyciem zmiennych lub współbieżnością nie występują. W ogólnym przypadku (poza testami jednostkowymi) nie wiadomo, czy dla każdego obiektu wywoływana jest co najwyżej jedna metoda.
Filip
1
@Philipp - nie kwestionuję problemu ponownego użycia lub współbieżności, ale pytanie OP dotyczyło wzoru, który jest zwykle używany w testach jednostkowych. Fakt, że urządzenie jest tworzone dla każdego testu, jest szczegółem implementacji środowiska testowego. Nawet w testach jednostkowych wzorzec jest bezpieczny tylko wtedy, gdy środowisko testowe jest używane tak, jak powinno być używane. To samo rozumowanie ma zastosowanie przy stosowaniu tego wzorca w kodzie produkcyjnym.
Lieven Keersmaekers
Nie ma potrzeby ustawiania barna null, JUnit i tak tworzy nową instancję testową dla każdej metody testowej.
oberlies
2

Ten zapach kodu jest określany przez Beck / Fowler jako pole tymczasowe w refaktoryzacji.

http://sourcemaking.com/refactoring/temporary-field

Edytowane w celu dodania dodatkowych wyjaśnień na prośbę moderatorów: Możesz przeczytać więcej o zapachu kodu w odnośniku powyżej lub w wersji papierowej książki. Ponieważ OP szukał nazwy tego szczególnego zapachu antypatternu / kodu, pomyślałem, że pomocne byłoby przytoczenie wcześniej wspomnianego odwołania z ustalonego źródła, które ma ponad 10 lat, choć w pełni zdaję sobie sprawę, że jestem spóźniony grę na to pytanie, więc jest mało prawdopodobne, że odpowiedź zostanie przegłosowana lub zaakceptowana.

Jeśli nie jest to zbyt osobiście promocyjne, odnoszę się i wyjaśniam ten szczególny zapach kodu na moim nadchodzącym kursie Pluralsight, Refactoring Fundamentals, który, jak się spodziewam, zostanie opublikowany w sierpniu 2013 r.

Aby zwiększyć wartość, porozmawiajmy o tym, jak rozwiązać ten konkretny problem. Istnieje kilka opcji. Jeśli pole jest rzeczywiście używane tylko przez jedną metodę, może zostać zdegradowane do zmiennej lokalnej. Jeśli jednak wiele metod używa pola do komunikacji (zamiast parametrów), jest to klasyczny przypadek opisany w Refaktoryzacji i można go poprawić, zastępując pole parametrami lub jeśli metody, które działają z tym kodem są ściśle powiązane, klasa Extract może być użyta do wyciągnięcia metod i pól z własnej klasy, w której pole jest zawsze w poprawnym stanie (być może ustawionym podczas budowy) i reprezentuje stan tej nowej klasy. Jeśli podążasz trasą parametrów, ale istnieje zbyt wiele wartości, aby je przekazać, inną opcją jest wprowadzenie nowego obiektu parametru,

ssmith
źródło
Należy zauważyć, że pola tymczasowe są często wymagane do refaktoryzacji klasy ekstraktów. Ale ktoś, kto jest tego świadomy, prawdopodobnie nie sprawdziłby kodu w tym stanie pośrednim.
oberlies
1

Powiedziałbym, że od razu do tego dojdzie, to tak naprawdę brak spójności, i mógłbym nadać mu nazwę „Fałszywa spójność”. Monetowałbym (lub jeśli skądś to biorę i zapominając o tym, „pożyczam”) ten termin, ponieważ klasa wydaje się być spójna, ponieważ jej metody działają na polu członkowskim. Jednak w rzeczywistości tak nie jest, co oznacza, że ​​pozorna spójność jest w rzeczywistości fałszywa.

Jeśli chodzi o to, czy jest źle, czy nie, powiedziałbym, że tak jest, i uważam, że Stephen C doskonale radzi sobie z wyjaśnianiem, dlaczego. Jednak niezależnie od tego, czy zasługuje na miano „anty-wzorca”, czy nie, myślę, że i jakikolwiek inny paradygmat kodowania może mieć związek z etykietą, ponieważ ogólnie ułatwia to komunikację.

Erik Dietrich
źródło
1

Oprócz wspomnianych już problemów, użycie tego podejścia w środowisku bez automatycznego wyrzucania elementów bezużytecznych (np. C ++) doprowadziłoby do wycieków pamięci, ponieważ obiekty tymczasowe nie są zwalniane po użyciu. (Choć może się to wydawać nieco zbyt daleko idące, istnieją ludzie, którzy po prostu zmieniliby „null” na „NULL” i byliby szczęśliwi, że kod się skompiluje.)

Peter
źródło
1

Wygląda mi to na okropne niewłaściwe zastosowanie wzoru Flyweight. Niestety znałem kilku programistów, którzy musieli wiele razy używać tej samej „rzeczy” na różne sposoby i po prostu wyciągać ją w prywatne pole, próbując zaoszczędzić pamięć lub poprawić wydajność lub inną dziwną pseudooptymalizację. Ich zdaniem próbują rozwiązać podobny problem, ponieważ Flyweight ma rozwiązać tylko to, że robią to w sytuacji, gdy tak naprawdę nie jest to problem wymagający rozwiązania.

Evicatos
źródło
-1

Zetknąłem się z tym wiele razy, zwykle podczas odświeżania kodu napisanego wcześniej przez kogoś, kto jako pierwszy tytuł wybrał VBA For Dummies i napisał stosunkowo duży system w jakimkolwiek języku, w którym się natknął.

Stwierdzenie, że jest to naprawdę zła praktyka programistyczna, jest słuszne, ale mogło to wynikać z braku zasobów internetowych i recenzowanych, takich jak wszyscy dzisiaj, którzy mogliby prowadzić oryginalnego programistę „bezpieczniejszą” ścieżką.

Jednak tak naprawdę nie zasługuje na tag „antipattern” - ale dobrze było zobaczyć sugestie dotyczące sposobu przekodowania OP.

Lee James
źródło
1
Witamy w Stack Exchange Programmers, dziękuję za wpisanie pierwszej odpowiedzi. Wygląda na to, że miałeś głos negatywny, być może z powodów wymienionych w FAQ programmers.stackexchange.com/faq . FAQ zawiera świetne sugestie dotyczące tworzenia skutecznych (i głosowanych) odpowiedzi. Czasami ludzie są na tyle uprzejmi, aby dodać notatkę o głosowaniu negatywnym, aby wskazać, czy jest to w jakiś sposób niewłaściwe, powielić, opinię bez dowodów lub też ja, który powtarza wcześniejszą odpowiedź. Większość z nas przegłosowała, zamknęła lub usunęła odpowiedzi, więc nie martw się. To tylko część rozpoczęcia pracy. Witamy.
DeveloperDon