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

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?

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.

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.

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.

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.

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

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).

(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

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.

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 .

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,

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ę.

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.)

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.

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.