Czy to zły pomysł, aby stworzyć metodę klasową, która jest przekazywana do zmiennych klasowych?

14

Oto co mam na myśli:

class MyClass {
    int arr1[100];
    int arr2[100];
    int len = 100;

    void add(int* x1, int* x2, int size) {
        for (int i = 0; i < size; i++) {
            x1[i] += x2[i];
        }
    }
};

int main() {
    MyClass myInstance;

    // Fill the arrays...

    myInstance.add(myInstance.arr1, myInstance.arr2, myInstance.len);
}

addmoże już uzyskać dostęp do wszystkich potrzebnych zmiennych, ponieważ jest to metoda klasowa, więc czy to zły pomysł? Czy istnieją powody, dla których powinienem lub nie powinienem tego robić?

papierowy mężczyzna
źródło
13
Jeśli masz ochotę to zrobić, oznacza to zły projekt. Właściwości klasy prawdopodobnie należą gdzie indziej.
bitsoflogic,
11
Fragment jest za mały. Ten poziom mieszanych abstrakcji nie występuje we fragmentach tego rozmiaru. Możesz to naprawić, dodając dobry tekst do rozważań projektowych.
Joshua
16
Naprawdę nie rozumiem, dlaczego to zrobiłeś. Co zyskujesz Dlaczego nie po prostu zastosować addmetodę bez argonu , która działa bezpośrednio na jej wewnętrznych elementach ? Dlaczego?
Ian Kemp,
2
@IanKemp Lub mieć addmetodę, która pobiera argumenty, ale nie istnieje jako część klasy. Tylko czysta funkcja dodawania dwóch tablic razem.
Kevin
Czy metoda add zawsze dodaje arr1 i arr2, czy może być używana do dodawania czegokolwiek?
user253751,

Odpowiedzi:

33

Z klasą mogą być rzeczy, które zrobiłbym inaczej, ale odpowiadając na bezpośrednie pytanie, moja odpowiedź byłaby

tak, to zły pomysł

Moim głównym powodem jest to, że nie masz kontroli nad tym, co jest przekazywane do addfunkcji. Pewnie masz nadzieję, że jest to jedna z tablic składowych, ale co się stanie, jeśli ktoś przejdzie w innej tablicy, która ma mniejszy rozmiar niż 100, lub jeśli przejdziesz przez długość większą niż 100?

Dzieje się tak, ponieważ stworzyłeś możliwość przekroczenia bufora. I to wszystko jest złe.

Aby odpowiedzieć na kilka (dla mnie) oczywistych pytań:

  1. Mieszasz tablice w stylu C z C ++. Nie jestem guru C ++, ale wiem, że C ++ ma lepsze (bezpieczniejsze) sposoby obsługi tablic

  2. Jeśli klasa ma już zmienne składowe, dlaczego musisz je przekazać? To bardziej pytanie architektoniczne.

Inne osoby z większym doświadczeniem w C ++ (przestałem go używać 10 lub 15 lat temu) mogą mieć bardziej wymowne sposoby wyjaśniania problemów i prawdopodobnie wymyślą również więcej problemów.

Peter M.
źródło
Rzeczywiście, vector<int>byłoby bardziej odpowiednie; długość byłaby wówczas bezużyteczna. Alternatywnie const int len, ponieważ tablica o zmiennej długości nie jest częścią standardu C ++ (chociaż niektóre kompilatory ją obsługują, ponieważ jest to opcjonalna funkcja w C).
Christophe
5
@Christophe Autor używał tablicy o stałym rozmiarze, którą należy zastąpić tablicą, która std::arraynie ulega rozkładowi podczas przekazywania jej do parametrów, ma wygodniejszy sposób na uzyskanie jej rozmiaru, jest kopiowalna i ma dostęp z opcjonalnym sprawdzaniem granic, nie powodując przy tym narzutu Tablice w stylu C.
Cubic
1
@Cubic: za każdym razem, gdy widzę kod deklarujący tablice o dowolnym ustalonym rozmiarze (np. 100), jestem prawie pewien, że autor jest początkujący, który nie ma pojęcia o implikacjach tego bzdury, i dobrze jest go polecić zmieniając ten projekt na coś o zmiennym rozmiarze.
Doc Brown,
Tablice są w porządku.
Wyścigi lekkości z Monicą
... dla tych, którzy nie rozumieli, co mam na myśli, zobacz także Zero One Infinity Rule
Doc Brown
48

Wywołanie metody klasy z niektórymi zmiennymi klasy niekoniecznie jest złe. Ale robienie tego spoza klasy jest bardzo złym pomysłem i sugeruje zasadniczą wadę w twoim projekcie OO, a mianowicie brak odpowiedniej enkapsulacji :

  • Każdy kod korzystający z twojej klasy musiałby wiedzieć, że lenjest to długość tablicy i konsekwentnie go używać. Jest to sprzeczne z zasadą najmniejszej wiedzy . Taka zależność od wewnętrznych szczegółów klasy jest wyjątkowo podatna na błędy i ryzykowna.
  • Utrudniłoby to ewolucję klasy (np. Gdybyś chciał zmienić starsze tablice i lenna bardziej nowoczesny std::vector<int>), ponieważ wymagałoby to również zmiany całego kodu za pomocą klasy.
  • Każda część kodu może siać spustoszenie w twoich MyClassobiektach, niszcząc niektóre publiczne zmienne bez przestrzegania reguł (tzw. Niezmienniki klas )
  • Wreszcie metoda jest w rzeczywistości niezależna od klasy, ponieważ działa tylko z parametrami i nie zależy od żadnego innego elementu klasy. Tego rodzaju metoda mogłaby równie dobrze być niezależną funkcją poza klasą. Lub przynajmniej metoda statyczna.

Powinieneś refaktoryzować swój kod i:

  • wprowadź zmienne klas privatelub protected, chyba że istnieje dobry powód, aby tego nie robić.
  • zaprojektuj metody publiczne jako działania do wykonania na klasie (np object.action(additional parameters for the action).:).
  • Jeśli po tym refaktoryzacji nadal będziesz mieć metody klasy, które należy wywołać za pomocą zmiennych klasy, uczyń je chronionymi lub prywatnymi po sprawdzeniu, że są to funkcje narzędziowe obsługujące metody publiczne.
Christophe
źródło
7

Jeśli celem tego projektu jest chęć ponownego użycia tej metody dla danych, które nie pochodzą z tego wystąpienia klasy, możesz zadeklarować ją jako staticmetodę .

Metoda statyczna nie należy do żadnej konkretnej instancji klasy. Jest to raczej metoda samej klasy. Ponieważ metody statyczne nie są uruchamiane w kontekście żadnej konkretnej instancji klasy, mogą one uzyskiwać dostęp tylko do zmiennych składowych, które są również zadeklarowane jako statyczne. Biorąc pod uwagę, że twoja metoda addnie odwołuje się do żadnej ze zmiennych składowych zadeklarowanych w MyClass, jest dobrym kandydatem do uzyskania deklaracji jako statyczna.

Jednak problemy bezpieczeństwa wymienione w innych odpowiedziach są ważne: Nie sprawdzasz, czy obie tablice są co najmniej lendługie. Jeśli chcesz pisać nowoczesny i solidny C ++, powinieneś unikać używania tablic. W std::vectormiarę możliwości używaj klasy . W przeciwieństwie do zwykłych tablic, wektory znają swój rozmiar. Więc nie musisz sam śledzić ich wielkości. Większość (nie wszystkie!) Ich metod wykonuje również automatyczne sprawdzanie granic, co gwarantuje, że otrzymasz wyjątek, gdy czytasz lub piszesz poza końcem. Regularny dostęp do tablicy nie wykonuje sprawdzania ograniczeń, co w najlepszym wypadku skutkuje segfaultem, a w gorszym przypadku może spowodować podatność na przepełnienie bufora .

Philipp
źródło
1

Metoda w klasie idealnie manipuluje enkapsulowanymi danymi wewnątrz klasy. W twoim przykładzie nie ma powodu, aby metoda add miała jakiekolwiek parametry, wystarczy użyć właściwości klasy.

Rik D.
źródło
0

Przekazywanie (części) obiektu do funkcji składowej jest w porządku. Wdrażając swoje funkcje, zawsze musisz mieć świadomość możliwego aliasingu i jego konsekwencji.

Oczywiście umowa może pozostawiać niektóre aliasy niezdefiniowane, nawet jeśli język to obsługuje.

Wybitne przykłady:

  • Samodzielne przydzielanie. Powinien to być, być może kosztowny, brak możliwości działania. Nie pesymalizuj powszechnego przypadku.
    Z drugiej strony zadanie samodzielnego poruszania się, chociaż nie powinno eksplodować w twoją twarz, nie musi być przeszkodą.
  • Wstawianie kopii elementu w kontenerze do kontenera. Coś jak v.push_back(v[2]);.
  • std::copy() zakłada, że ​​miejsce docelowe nie zaczyna się w źródle.

Ale twój przykład ma inne problemy:

  • Funkcja członka nie korzysta z żadnych swoich uprawnień ani się do niej nie odwołuje this. Działa jak darmowa funkcja użyteczności, która po prostu znajduje się w niewłaściwym miejscu.
  • Twoja klasa nie próbuje przedstawiać żadnej abstrakcji . Zgodnie z tym, nie próbuje on zamaskować swoich wnętrzności, ani utrzymywać żadnych niezmienników . To głupia torba z dowolnymi elementami.

Podsumowując: Tak, ten przykład jest zły. Ale nie dlatego, że funkcja członka otrzymuje przekazywane elementy członkowskie.

Deduplikator
źródło
0

W szczególności zrobienie tego jest złym pomysłem z kilku dobrych powodów, ale nie jestem pewien, czy wszystkie z nich są integralną częścią twojego pytania:

  • Posiadanie zmiennych klas (zmiennych rzeczywistych, a nie stałych) jest czymś, czego w ogóle można uniknąć, i powinno poważnie zastanowić się, czy jest to absolutnie potrzebne.
  • Pozostawienie dostępu do zapisu do tych zmiennych klas całkowicie otwartych dla wszystkich jest prawie ogólnie złe.
  • Twoja metoda klasowa nie ma związku z twoją klasą. W rzeczywistości jest to funkcja, która dodaje wartości jednej tablicy do wartości innej tablicy. Ten rodzaj funkcji powinien być funkcją w języku, który ma funkcje, i powinien być statyczną metodą „fałszywej klasy” (tj. Zbioru funkcji bez zachowania danych lub obiektów) w językach, które nie mają funkcji.
  • Alternatywnie, usuń te parametry i zmień je w metodę prawdziwej klasy, ale nadal byłoby złe z wyżej wymienionych powodów.
  • Dlaczego tablice int? vector<int>ułatwiłoby ci życie.
  • Jeśli ten przykład jest naprawdę reprezentatywny dla twojego projektu, rozważ umieszczenie danych w obiektach, a nie w klasach, a także zaimplementowanie konstruktorów dla tych obiektów w sposób, który nie wymaga zmiany wartości danych obiektu po utworzeniu.
Kafein
źródło
0

Jest to klasyczne podejście „C z klasami” do C ++. W rzeczywistości nie tak napisałby każdy doświadczony programista C ++. Po pierwsze, używanie surowych tablic C jest ogólnie odradzane, chyba że implementujesz bibliotekę kontenerów.

Coś takiego byłoby bardziej odpowiednie:

// Don't forget to compile with -std=c++17
#include <iostream>
using std::cout; // This style is less common, but I prefer it
using std::endl; // I'm sure it'll incite some strongly opinionated comments

#include <array>
using std::array;

#include <algorithm>

#include <vector>
using std::vector;


class MyClass {
public: // Begin lazy for brevity; don't do this
    std::array<int, 5> arr1 = {1, 2, 3, 4, 5};
    std::array<int, 5> arr2 = {10, 10, 10, 10, 10};
};

void elementwise_add_assign(
    std::array<int, 5>& lhs,
    const std::array<int, 5>& rhs
) {
    std::transform(
        lhs.begin(), lhs.end(), rhs.begin(),
        lhs.begin(),
        [](auto& l, const auto& r) -> int {
            return l + r;
        });
}

int main() {
    MyClass foo{};

    elementwise_add_assign(foo.arr1, foo.arr2);

    for(auto const& value: foo.arr1) {
        cout << value << endl; // arr1 values have been added to
    }

    for(auto const& value: foo.arr2) {
        cout << value << endl; // arr2 values remain unaltered
    }
}
Alexander - Przywróć Monikę
źródło