E 'una cattiva idea avere un metodo di classe che passi le variabili di classe?

Potrebbero esserci cose con la classe che farei diversamente, ma per rispondere alla domanda diretta, la mia risposta sarebbe

sì, è una cattiva idea

Il mio motivo principale è che non hai il controllo su ciò che viene passato alla funzione add . Certo, speri che sia uno degli array membri, ma cosa succede se qualcuno passa in un array diverso che ha una dimensione inferiore a 100 o se superi una lunghezza superiore a 100?

Quello che succede è che hai creato la possibilità di un sovraccarico del buffer. E questa è una brutta cosa tutt'intorno.

Rispondere ad alcune (ovvio) domande ovvie:

1. Stai mescolando gli array in stile C con C ++. Non sono un guru del C ++, ma lo faccio sappi che C ++ ha modi migliori (più sicuri) di gestire gli array

2. Se il la classe ha già le variabili membro, perché è necessario passarle nel? Questa è più una questione architettonica.

Altre persone con più esperienza C ++ (ho smesso di usarlo 10 o 15 anni fa) potrebbero avere modi più eloquenti di spiegare i problemi, e probabilmente genereranno anche più problemi.

Chiamare un metodo di classe con alcune variabili di classe non è necessariamente negativo. Ma farlo al di fuori della classe è una pessima idea e suggerisce un difetto fondamentale nel tuo design OO, vale a dire l'assenza di una corretta incapsulamento :

• Qualsiasi codice che usi la tua classe dovrebbe sapere che len è la lunghezza dell'array e usarlo in modo coerente. Ciò va contro il principio della minima conoscenza . Tale dipendenza dai dettagli interni della classe è estremamente soggetta a errori e rischiosa.
• Ciò renderebbe molto difficile l'evoluzione della classe (ad esempio se un giorno vorresti cambiare gli array legacy e len a un std::vector<int> più moderno), poiché richiederebbe anche di cambiare tutto il codice usando la tua classe.
• Qualsiasi parte di codice può provocare il caos nei tuoi oggetti MyClass corrompendo alcune variabili pubbliche senza rispettare le regole (i cosiddetti invarianti di classe )
• Infine, il metodo è in realtà indipendente dalla classe, poiché funziona solo con i parametri e non dipende da nessun altro elemento di classe. Questo tipo di metodo potrebbe benissimo essere una funzione indipendente al di fuori della classe. O almeno un metodo statico.

Devi refactoring il tuo codice e:

• imposta le tue variabili di classe private o protected , a meno che non ci sia un buon motivo per non farlo.
• progetta i tuoi metodi pubblici come azioni da eseguire sulla classe (ad esempio object.action(additional parameters for the action) ).
• Se dopo questo refactoring, avresti ancora alcuni metodi di classe che devono essere chiamati con le variabili di classe, renderli protetti o privati dopo aver verificato che si tratta di funzioni di utilità che supportano i metodi pubblici.

Quando l'intenzione di questo disegno è quella di voler riutilizzare questo metodo per i dati che non provengono da questa istanza di classe, si potrebbe voler dichiararlo come static method .

Un metodo statico non appartiene ad alcuna istanza particualar di una classe. È piuttosto un metodo della classe stessa. Poiché i metodi statici non vengono eseguiti nel contesto di una particolare istanza della classe, possono solo accedere alle variabili membro che sono anche dichiarate come statiche. Considerando che il tuo metodo add non fa riferimento a nessuna delle variabili membro dichiarate in MyClass , è un buon candidato per essere dichiarato statico.

Tuttavia, i problemi di sicurezza menzionati dalle altre risposte sono validi: non stai controllando se entrambi gli array sono almeno len di lunghezza. Se vuoi scrivere C ++ moderno e robusto, dovresti evitare di usare gli array. Utilizza invece la classe std::vector quando possibile. Contrariamente agli array regolari, i vettori conoscono le proprie dimensioni. Quindi non è necessario tenere traccia delle proprie dimensioni da soli. La maggior parte (non tutti!) Dei loro metodi esegue anche il controllo automatico dei limiti, che garantisce di ottenere un'eccezione quando si legge o si scrive oltre la fine. L'accesso regolare agli array non esegue il controllo vincolato, il che si traduce in un segfault nella migliore delle ipotesi e potrebbe causare una vulnerabilità di buffer overflow sfruttabile a peggio.

Il passaggio (parte di) di un oggetto a una funzione membro va bene. Quando implementi le tue funzioni, sempre devi essere consapevole del possibile aliasing e delle relative conseguenze.

Naturalmente, il contratto potrebbe lasciare alcuni tipi di alias non definiti anche se il linguaggio lo supporta.

Esempi di spicco:

• Autoassegnazione. Questo dovrebbe essere un, possibilmente costoso, no-op. Non pessimizzare il caso comune per questo.
  L'auto-spostamento-assegnazione invece, mentre non dovrebbe esplodere in faccia, non deve essere un no-op.
• Inserimento di una copia di un elemento in un contenitore nel contenitore. Qualcosa come v.push_back(v[2]); .
• std::copy() presuppone che la destinazione non inizi all'interno della sorgente.

Ma il tuo esempio ha diversi problemi:

• La funzione membro non usa nessuno dei suoi privilegi, né fa riferimento a this . Funziona come una funzione di utilità gratuita che si trova semplicemente nel posto sbagliato.
• La tua classe non tenta di presentare alcun tipo di astrazione . In linea con questo, non prova a incapsulare i suoi interni, né a mantenere alcun invarianti . È un sacco di elementi arbitrari.

In breve: Sì, questo esempio è sbagliato. Ma non perché la funzione membro venga passata agli oggetti membro.

In particolare, fare ciò è una cattiva idea per diversi buoni motivi, ma non sono sicuro che siano tutti parte integrante della tua domanda:

• Avere variabili di classe (variabili reali, non costanti) è qualcosa da evitare se possibile, e dovrebbe innescare una seria riflessione sul fatto che sia assolutamente necessario.
• Lasciando che l'accesso in scrittura a queste variabili di classe completamente aperte a tutti è quasi universalmente sbagliato.
• Il tuo metodo di classe non è correlato alla tua classe. Ciò che realmente è è una funzione che aggiunge i valori di una matrice ai valori di un'altra matrice. Questo tipo di funzione dovrebbe essere una funzione in un linguaggio che ha funzioni e dovrebbe essere un metodo statico di una "classe falsa" (ovvero una raccolta di funzioni senza dati o comportamento di oggetti) in lingue che non hanno funzioni.
• In alternativa, rimuovi questi parametri e trasformali in un vero e proprio metodo di classe, ma sarebbe comunque negativo per i motivi indicati in precedenza.
• Perché gli array int sono? vector<int> ti renderebbe la vita più facile.
• Se questo esempio è davvero rappresentativo del tuo progetto, considera la possibilità di inserire i dati in oggetti e non nelle classi e di implementare i costruttori per questi oggetti in un modo che non richiede la modifica del valore dei dati dell'oggetto dopo la creazione.

Questo è un classico approccio "C con classi" a C ++. In realtà, questo non è ciò che qualsiasi programmatore C ++ esperto avrebbe scritto. Per esempio, l'utilizzo di array C grezzi è quasi universalmente scoraggiato, a meno che non si stia implementando una libreria container.

Qualcosa di simile sarebbe più appropriato:

// 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
    }
}