Przekazywanie zmiennej elementu jako parametru metody

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?

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 .

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

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.

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.

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.

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

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.

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 .

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.

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