Przekazywanie zmiennej elementu jako parametru metody

33

W projekcie znalazłem taki kod:

class SomeClass
{
    private SomeType _someField;

    public SomeType SomeField
    {
        get { return _someField; }
        set { _someField = value; }
    }

    protected virtual void SomeMethod(/*...., */SomeType someVar)
    {
    }

    private void SomeAnotherMethod()
    {
        //.............
        SomeMethod(_someField);
        //.............
    }

};

Jak przekonać kolegów z drużyny, że to zły kod?

Uważam, że jest to niepotrzebna komplikacja. Po co przekazywać zmienną składową jako parametr metody, jeśli masz już do niej dostęp? Jest to również naruszenie enkapsulacji.

Czy widzisz jakieś inne problemy z tym kodem?

tika
źródło
21
Co sprawia, że ​​myślisz, że to źle?
yannis
@Yannis Rizos, myślisz, że to dobrze? Przynajmniej jest to niepotrzebna komplikacja. Po co przekazywać zmienną jako parametr metody, jeśli masz już do niej dostęp? Jest to również naruszenie enkapsulacji.
tika
2
Ważne punkty, edytuj pytanie, aby je uwzględnić. Nie możemy pomóc Ci przekonać kolegów z zespołu, ale możemy pomóc ci ocenić kod, o to powinno być twoje pytanie.
yannis
8
Wolę metodę, która może sumować inne zmienne niż metodę, która wykonuje stałą 2 + 2. Parametr w metodzie służy do ponownego użycia.
Dante
Jednym z punktów, który moim zdaniem jest ważny, jest rodzaj tego parametru. Jeśli jest to typ referencyjny, nie widzę żadnej korzyści, ale jeśli jest to typ wartości, myślę, że ma to jakiś sens, ponieważ jeśli zmodyfikujesz ten typ zmiennej, kompilator ostrzeże Cię o miejscu, w którym złamałeś kod.
Rémi

Odpowiedzi:

3

Myślę, że jest to ważny temat, ale otrzymujesz mieszane odpowiedzi ze względu na sposób, w jaki powstało pytanie. Osobiście miałem te same doświadczenia ze swoim zespołem, w których przekazywanie członków jako argumentów było niepotrzebne i zawijało kod. Mielibyśmy klasę, która działa z zestawem elementów, ale niektóre funkcje mają bezpośredni dostęp do elementów, a inne funkcje modyfikują te same elementy za pomocą parametrów (tj. Używają zupełnie innych nazw) i nie ma absolutnie żadnego technicznego powodu. Z technicznego punktu widzenia mam na myśli przykład podany przez Kate.

Radziłbym cofnąć się o krok i zamiast skupiać się na przekazywaniu członków jako parametrach, zainicjuj dyskusje ze swoim zespołem na temat przejrzystości i czytelności. Albo bardziej formalnie, albo tylko na korytarzach, przedyskutuj, co sprawia, że ​​niektóre segmenty kodu są łatwiejsze do odczytania, a inne segmenty kodu trudniejsze. Następnie określ miary jakości lub atrybuty czystego kodu, o które jako zespół chciałbyś dążyć. W końcu, nawet podczas pracy nad projektami typu green field, spędzamy ponad 90% czasu na czytaniu i jak tylko kod zostanie napisany (powiedzmy 10-15 minut później), przechodzi on do konserwacji, gdzie czytelność jest jeszcze ważniejsza.

Tak więc w twoim konkretnym przykładzie użyłbym argumentu, że mniej kodu jest zawsze łatwiejsze do odczytania niż więcej kodu. Funkcja, która ma 3 parametry, jest trudniejsza do przetworzenia dla mózgu niż funkcja, która nie ma żadnego lub jednego parametru. Jeśli istnieje inna nazwa zmiennej, mózg musi śledzić jeszcze jedną rzecz podczas czytania kodu. Więc pamiętajmy „int m_value”, a następnie „int localValue” i pamiętajmy, że jeden naprawdę oznacza, że ​​drugi jest zawsze droższy dla mózgu niż praca z „m_value”.

Aby uzyskać więcej amunicji i pomysłów, polecam zabranie kopii Czystego Kodu Wujka Boba .

DXM
źródło
Przegłosowano po zobaczeniu wpływu wspomnianej książki.
Frank Hileman,
Chociaż niewielka część mnie jest bardzo smutna, że ​​5 lat po napisaniu odpowiedzi zabrałeś mi 2 punkty internetowe, jest jeszcze jedna niewielka część mnie, która jest ciekawa, czy możesz podać jakieś referencje wskazujące na (prawdopodobnie złe) wpływać na książkę, o której wspominałem. Wydawałoby się to uczciwe i prawie warte tych punktów
DXM
referencja to ja, widząc osobiście wpływ na innych programistów. Wszystkie motywy leżące u podstaw takich książek są jednak dobre, ponieważ określając, że cały kod musi być zgodny z niestandardowymi wytycznymi (tj. Wytycznymi, które faktycznie służą celowi), książki takie wydają się powodować utratę krytycznego myślenia, które niektórzy interpretują jako kultowe .
Frank Hileman
jeśli chodzi o wyzwania związane z przetwarzaniem mózgu, rozważ koncepcję obciążenia poznawczego
jxramos
30

Mogę myśleć o uzasadnieniu przekazania pola członka jako parametru w metodzie (prywatnej): wyraźnie określa, od czego zależy twoja metoda.

Jak mówisz, wszystkie pola składowe są niejawnymi parametrami twojej metody, ponieważ jest to cały obiekt. Czy jednak pełny obiekt jest naprawdę potrzebny do obliczenia wyniku? Jeśli SomeMethodjest to metoda wewnętrzna, która zależy tylko od tego _someField, czy nie jest bardziej przejrzyste, aby ta zależność była wyraźna? W rzeczywistości wyraźne zdefiniowanie tej zależności może również sugerować, że można zrefaktoryzować ten fragment kodu poza klasę! (Uwaga: zakładam, że nie mówimy tutaj o programach pobierających ani ustawiających, ale kod, który faktycznie coś oblicza)

Nie podałbym tego samego argumentu dla metod publicznych, ponieważ program wywołujący nie wie ani nie dba o to, która część obiektu jest istotna do obliczenia wyniku ...

Andres F.
źródło
2
Jaka byłaby pozostała domniemana zależność? Z twojego przykładu założyłem, że _someFieldjest to jedyny parametr potrzebny do obliczenia wyniku, i właśnie wyraziliśmy to jasno. (. Uwaga: to jest ważne Nie dodawać zależność, że staje się on wyraźny!)
Andres F.
11
-1 Jeśli nie ma żadnych ukrytych zależności między elementami instancji, powinien to być element statyczny, który przyjmuje wartość jako parametr. W tym przypadku, chociaż przedstawiłeś powód, nie sądzę, aby było to uzasadnione uzasadnienie. To zły kod.
Steven Evers
2
@SnOrfus Właściwie zasugerowałem refaktoryzację metody całkowicie poza klasą
Andres F.,
5
+1. dla „... czy nie jest bardziej przejrzyste wyrażanie tej zależności?” Abso-cholernie-loutely. Kto by powiedziałby, że „zmienne globalne są z reguły dobre”? To jest tak COBOL-68. Posłuchaj mnie teraz i uwierz mi później. W naszym nietrywialnym kodzie będę czasami refaktoryzować, aby wyraźnie przekazać, gdzie używane są zmienne globalne klasy. W wielu przypadkach psowaliśmy psiaka przez a) arbitralne korzystanie z prywatnego pola i jego własności publicznej b) zaciemnianie transformacji pól przez „ukrywanie zależności”. Teraz pomnóż to przez 3-5 głębokiego łańcucha dziedziczenia.
radarbob
2
@Tarion Nie zgadzam się z wujkiem Bobem w tej sprawie. O ile to możliwe, metody powinny być podobne do funkcji i zależeć tylko od wyraźnych zależności. (Podczas wywoływania metod publicznych w OOP jedna z tych zależności jest this(lub self), ale jest to wyraźnie zaznaczone przez samo wywołanie obj.method(x)). Inne niejawne zależności to stan obiektu; zwykle utrudnia to zrozumienie kodu. Gdy tylko jest to możliwe - i w granicach rozsądku - wyraźne zależności i styl funkcjonalny. W przypadku metod prywatnych, jeśli to możliwe, jawnie przekaż każdy potrzebny parametr. I tak, pomaga je refaktoryzować.
Andres F.,
28

Widzę jeden silny powód, aby przekazywać zmienne składowe jako argumenty funkcji do metod prywatnych - czystość funkcji. Zmienne składowe są faktycznie stanem globalnym z punktu widzenia, a ponadto zmiennym stanem globalnym, jeśli wymieniony element zmienia się podczas wykonywania metody. Zastępując referencje zmiennych składowych parametrami metody, możemy skutecznie uczynić funkcję czystą. Czyste funkcje nie zależą od stanu zewnętrznego i nie powodują żadnych skutków ubocznych, zawsze zwracają te same wyniki, biorąc pod uwagę ten sam zestaw parametrów wejściowych - co ułatwia testowanie i konserwację w przyszłości.

Oczywiście nie jest łatwo ani praktyczne, aby wszystkie metody były czystymi metodami w języku OOP. Sądzę jednak, że zyskujesz znacznie na przejrzystości kodu dzięki metodom, które obsługują złożoną logikę czystą i używają niezmiennych zmiennych, przy jednoczesnym zachowaniu nieczystej „globalnej” obsługi stanu oddzielonej w ramach dedykowanych metod.

Przekazywanie zmiennych składowych do funkcji publicznej tego samego obiektu podczas wywoływania funkcji na zewnątrz stanowiłoby jednak moim zdaniem poważny zapach kodu.

scrwtp
źródło
5
Wspaniała odpowiedź. Pomimo tego, że hwats jest idealny, wiele klas dzisiejszego oprogramowania jest większych i brzydszych niż całe programy, kiedy powstała faza „Globale są złe”. Do wszystkich praktycznych celów zmienne klasowe są globalne w ramach instancji klasy. Wykonanie dużej ilości pracy w czystych funkcjach zapewnia znacznie bardziej testowalny i solidny kod.
mattnz
@mattnz czy jest jakiś sposób na podanie linku do bibliografii Hwata lub jego książek o idealnym programowaniu? Szukałem w Internecie i nie mogę nic na nim znaleźć. Google stara się autokorektywać to na „co”.
Buttle Butkus
8

Jeśli funkcja jest wywoływana w różnych momentach, czasami przekazując tę ​​zmienną składową, a czasami przekazując coś innego, wtedy jest OK. Na przykład wcale nie uważałbym tego za złe:

if ( CalculateCharges(newStartDate) > CalculateCharges(m_StartDate) )
{
     //handle increase in charges
}

gdzie newStartDatejest jakaś zmienna lokalna i m_StartDatejest zmienną składową.

Jeśli jednak funkcja zostanie wywołana tylko wtedy, gdy zmienna członkowska zostanie do niej przekazana, to dziwne. Funkcje składowe działają przez cały czas na zmiennych składowych. Być może robią to (w zależności od języka, w którym pracujesz), aby uzyskać kopię zmiennej składowej - jeśli tak jest i nie możesz tego zobaczyć, kod może być ładniejszy, jeśli wyrazi cały proces.

Kate Gregory
źródło
3
Nie ma znaczenia, że ​​metoda jest wywoływana z parametrami innymi niż zmienna składowa. Liczy się to, że można to tak nazwać. Nie zawsze wiesz, jak metoda zostanie ostatecznie wywołana podczas jej tworzenia.
Caleb
Być może powinieneś lepiej zastąpić warunek „if” przez „needHandleIncreaseChages (newStartDate)”, a wtedy twój argument już się nie utrzymuje.
Tarion
2

Nikt się nie dotknął, że SomeMethod jest chroniony wirtualnie. Oznacza to, że klasa pochodna może z niej korzystać ORAZ ponownie wdrożyć swoją funkcjonalność. Klasa pochodna nie miałaby dostępu do zmiennej prywatnej, a zatem nie mogłaby zapewnić niestandardowej implementacji SomeMethod zależnej od zmiennej prywatnej. Zamiast przejmować zależność od zmiennej prywatnej, deklaracja wymaga od osoby dzwoniącej przekazania jej.

Michael Brown
źródło
Brakuje ci tylko tego, że ta zmienna prywatna członek ma publicznych akcesorów.
tika
Więc mówisz, że chroniona metoda wirtualna powinna zależeć od publicznego akcesora zmiennej prywatnej? Masz problem z bieżącym kodem?
Michael Brown
Przystawki publiczne są tworzone do użycia. Kropka.
tika
1

Frameworki GUI zazwyczaj mają jakąś klasę „Widok”, która reprezentuje rzeczy narysowane na ekranie, i ta klasa zazwyczaj zapewnia metodę na przykład invalidateRect(Rect r)zaznaczenia części obszaru rysowania jako wymagającej przerysowania. Klienci mogą wywołać tę metodę, aby zażądać aktualizacji części widoku. Ale widok może również wywoływać własną metodę, na przykład:

invalidateRect(m_frame);

spowodować przerysowanie całego obszaru. Na przykład może to zrobić, gdy zostanie po raz pierwszy dodany do hierarchii widoku.

Nie ma w tym nic złego - ramka widoku jest poprawnym prostokątem, a sam widok wie, że chce się narysować. Klasa View może udostępnić osobną metodę, która nie przyjmuje parametrów i używa zamiast tego ramki widoku:

invalidateFrame();

Ale po co dodawać do tego specjalną metodę, skoro można użyć bardziej ogólnej invalidateRect()? Lub, jeśli zdecydujesz się na dostarczenie invalidateFrame(), najprawdopodobniej zaimplementujesz go w kategoriach bardziej ogólnych invalidateRect():

View::invalidateFrame(void)
{
    invalidateRect(m_frame)
}

Po co przekazywać zmienną jako parametr metody, jeśli masz już do niej dostęp?

Państwo powinno przekazać zmienne instancji jako parametry do własnej metody , jeśli metoda nie działa w szczególności na tej zmiennej instancji. W powyższym przykładzie ramka widoku jest tylko kolejnym prostokątem, jeśli chodzi o invalidateRect()metodę.

Caleb
źródło
1

Ma to sens, jeśli metoda jest metodą użytkową. Powiedz na przykład, że musisz uzyskać unikalną krótką nazwę z kilku ciągów dowolnego tekstu.

Nie chcesz kodować osobnej implementacji dla każdego łańcucha, zamiast tego przekazanie łańcucha do wspólnej metody ma sens.

Jednak jeśli metoda zawsze działa na pojedynczym elemencie, wydaje się trochę głupim przekazywanie jej jako parametru.

James Anderson
źródło
1

Głównym powodem posiadania zmiennej członka w klasie jest umożliwienie jej ustawienia w jednym miejscu i udostępnienia jej wartości każdej innej metodzie w klasie. Ogólnie rzecz biorąc, można oczekiwać, że nie ma potrzeby przekazywania zmiennej składowej do metody klasy.

Mogę jednak wymyślić kilka powodów, dla których możesz chcieć przekazać zmienną składową do innej metody klasy. Po pierwsze, jeśli musisz zagwarantować, że wartość zmiennej składowej musi zostać użyta w niezmienionej formie, gdy jest używana z wywoływaną metodą, nawet jeśli ta metoda będzie musiała zmienić rzeczywistą wartość zmiennej składowej w pewnym momencie procesu. Drugi powód jest związany z pierwszym, ponieważ możesz chcieć zagwarantować niezmienność wartości w ramach łańcucha metod - na przykład podczas implementacji płynnej składni.

Mając to wszystko na uwadze, nie posunąłbym się nawet do stwierdzenia, że ​​kod jest „zły” jako taki, jeśli przekażesz zmienną składową do jednej z metod klasy. Sugerowałbym jednak, że generalnie nie jest idealny, ponieważ może zachęcać do dużego duplikowania kodu, gdy parametr jest przypisany na przykład do zmiennej lokalnej, a dodatkowe parametry dodają „szum” w kodzie, gdzie nie jest potrzebny. Jeśli jesteś fanem książki Clean Code, wiesz, że wspomina ona o tym, że należy ograniczyć liczbę parametrów metody do jak najmniejszego minimum i tylko wtedy, gdy nie ma innego bardziej sensownego sposobu na uzyskanie dostępu do parametru .

S.Robins
źródło
1

Kilka rzeczy, które przychodzą mi do głowy, gdy o tym myślę:

  1. Ogólnie, podpisy metod o mniejszej liczbie parametrów są łatwiejsze do zrozumienia; głównym powodem wynalezienia metod było wyeliminowanie długich list parametrów, łącząc je z danymi, na których działają.
  2. Uzależnienie podpisu metody od zmiennej elementu spowoduje, że trudniej będzie zmienić te zmienne w przyszłości, ponieważ nie tylko trzeba będzie zmieniać metodę, ale wszędzie tam, gdzie metoda jest wywoływana. A ponieważ SomeMethod w twoim przykładzie jest chroniony, należy również zmienić podklasy.
  3. Metody (publiczne lub prywatne), które nie zależą od wewnętrznych elementów klasy, nie muszą być w tej klasie. Można je rozdzielić na metody użytkowe i być równie szczęśliwym. Nie mają prawie żadnej firmy należącej do tej klasy. Jeśli żadna inna metoda nie zależy od tej zmiennej po przeniesieniu metody (metod), to ta zmienna również powinna przejść! Najprawdopodobniej zmienna powinna znajdować się na swoim obiekcie, a ta metoda lub metody, które działają na niej, stają się publiczne i składają się z klasy nadrzędnej.
  4. Przekazywanie danych do różnych funkcji, takich jak klasa, jest programem proceduralnym, w którym zmienne globalne po prostu rzucają się w oczy w obliczu projektu OO. Nie takie jest przeznaczenie zmiennych składowych i funkcji składowych (patrz wyżej) i brzmi to tak, jakby twoja klasa nie była zbyt spójna. Myślę, że to zapach kodu sugerujący lepszy sposób grupowania danych i metod.
Colin Hunt
źródło
0

Brakuje, jeśli parametr („argument”) jest referencyjny lub tylko do odczytu.

class SomeClass
{
    protected SomeType _someField;
    public SomeType SomeField
    {
        get { return _someField; }

        set {
          if (doSomeValidation(value))
          {
            _someField = value;
          }
        }
    }

    protected virtual void ModifyMethod(/*...., */ ref SomeType someVar)
    { 
      // ...
    }    

    protected virtual void ReadMethod(/*...., */ SomeType someVar)
    { 
      // ...
    }

    private void SomeAnotherMethod()
    {
        //.............

        // not recommended, but, may be required in some cases
        ModifyMethod(ref this._someField);

        //.............

        // recommended, but, verbose
        SomeType SomeVar = this.someField;
        ModifyMethod(ref SomeVar);
        this.someField = SomeVar;

        //.............

        ReadMethod(this.someField);
        //.............
    }

};

Wielu programistów zwykle przypisuje bezpośrednio wewnętrzne pola zmiennej w metodach konstruktora. Jest kilka wyjątków.

Pamiętaj, że „ustawiacze” mogą mieć dodatkowe metody, nie tylko przypisanie, a czasem nawet metody wirtualne lub wywoływać metody wirtualne.

Uwaga: Proponuję zachować wewnętrzne pola właściwości jako „chronione”.

umlcat
źródło