Est-ce une mauvaise idée de créer une méthode de classe avec des variables de classe transmises?

Voici ce que je veux dire:

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);
}

addpeut déjà accéder à toutes les variables dont il a besoin, car il s'agit d'une méthode de classe, est-ce donc une mauvaise idée? Y a-t-il des raisons pour lesquelles je devrais ou ne devrais pas faire cela?

Il y a peut-être des choses avec la classe que je ferais différemment, mais pour répondre à la question directe, ma réponse serait

oui, c'est une mauvaise idée

Ma principale raison est que vous n'avez aucun contrôle sur ce qui est transmis à la addfonction. Bien sûr, vous espérez qu'il s'agit de l'un des tableaux membres, mais que se passe-t-il si quelqu'un passe dans un tableau différent qui a une taille inférieure à 100, ou si vous passez dans une longueur supérieure à 100?

Ce qui se passe, c'est que vous avez créé la possibilité d'un dépassement de tampon. Et c'est une mauvaise chose tout autour.

Pour répondre à d'autres questions (pour moi) évidentes:

1. Vous mélangez des tableaux de style C avec C ++. Je ne suis pas un gourou du C ++, mais je sais que le C ++ a de meilleures façons (plus sûres) de gérer les tableaux

2. Si la classe a déjà les variables membres, pourquoi avez-vous besoin de les transmettre? C'est plus une question architecturale.

D'autres personnes ayant plus d'expérience en C ++ (j'ai cessé de l'utiliser il y a 10 ou 15 ans) peuvent avoir des façons plus éloquentes d'expliquer les problèmes et trouveront probablement plus de problèmes également.

L'appel d'une méthode de classe avec certaines variables de classe n'est pas nécessairement mauvais. Mais le faire de l'extérieur de la classe est une très mauvaise idée et suggère une faille fondamentale dans votre conception OO, à savoir l'absence d' encapsulation appropriée :

• Tout code utilisant votre classe devrait connaître lenla longueur du tableau et l'utiliser de manière cohérente. Cela va à l'encontre du principe du moindre savoir . Une telle dépendance à l'égard des détails internes de la classe est extrêmement sujette aux erreurs et risquée.
• Cela rendrait l'évolution de la classe très difficile (par exemple, si un jour, vous souhaitez changer les tableaux hérités et les lenrendre plus modernes std::vector<int>), car cela vous obligerait à changer également tout le code en utilisant votre classe.
• N'importe quelle partie du code pourrait faire des ravages dans vos MyClassobjets en corrompant certaines variables publiques sans respecter les règles (appelées invariants de classe )
• Enfin, la méthode est en réalité indépendante de la classe, car elle ne fonctionne qu'avec les paramètres et ne dépend d'aucun autre élément de classe. Ce type de méthode pourrait très bien être une fonction indépendante en dehors de la classe. Ou au moins une méthode statique.

Vous devez refactoriser votre code et:

• créez vos variables de classe privateou protected, à moins qu'il n'y ait une bonne raison de ne pas le faire.
• concevez vos méthodes publiques comme des actions à effectuer sur la classe (par exemple:) object.action(additional parameters for the action).
• Si après cette refactorisation, vous auriez encore des méthodes de classe qui doivent être appelées avec des variables de classe, rendez-les protégées ou privées après avoir vérifié qu'elles sont des fonctions utilitaires prenant en charge les méthodes publiques.

Lorsque l'intention de cette conception est que vous souhaitez pouvoir réutiliser cette méthode pour des données qui ne proviennent pas de cette instance de classe, vous pouvez alors la déclarer en tant que staticméthode .

Une méthode statique n'appartient à aucune instance particulière d'une classe. C'est plutôt une méthode de la classe elle-même. Étant donné que les méthodes statiques ne sont pas exécutées dans le contexte d'une instance particulière de la classe, elles ne peuvent accéder qu'aux variables membres qui sont également déclarées comme statiques. Étant donné que votre méthode addne fait référence à aucune des variables membres déclarées dans MyClass, elle est un bon candidat pour être déclarée comme statique.

Cependant, les problèmes de sécurité mentionnés par les autres réponses sont valides: vous ne vérifiez pas si les deux baies sont au moins lenlongues. Si vous souhaitez écrire du C ++ moderne et robuste, vous devez éviter d'utiliser des tableaux. Utilisez std::vectorplutôt la classe autant que possible. Contrairement aux tableaux réguliers, les vecteurs connaissent leur propre taille. Vous n'avez donc pas besoin de suivre vous-même leur taille. La plupart (pas toutes!) De leurs méthodes font également une vérification automatique des liens, ce qui garantit que vous obtenez une exception lorsque vous lisez ou écrivez après la fin. L'accès régulier aux baies ne fait pas de vérification des limites, ce qui entraîne au mieux une erreur de segmentation et peut, au pire, entraîner une vulnérabilité de dépassement de tampon exploitable .

Une méthode dans une classe manipule idéalement des données encapsulées à l'intérieur de la classe. Dans votre exemple, il n'y a aucune raison pour que la méthode add ait des paramètres, utilisez simplement les propriétés de la classe.

Passer (une partie de) un objet à une fonction membre est très bien. Lors de l' implémentation de vos fonctions, vous toujours devez être conscient de l' aliasing possible , et les conséquences de celle - ci.

Bien sûr, le contrat peut laisser certains types d'alias indéfinis même si la langue le prend en charge.

Exemples marquants:

• Auto-affectation. Cela devrait être un no-op, éventuellement coûteux. Ne pessimisez pas le cas commun pour cela.
  L'auto-affectation de mouvement, d'autre part, bien qu'elle ne devrait pas exploser dans votre visage, n'a pas besoin d'être un non-op.
• Insertion d'une copie d'un élément d'un conteneur dans le conteneur. Quelque chose comme v.push_back(v[2]);.
• std::copy() suppose que la destination ne commence pas à l'intérieur de la source.

Mais votre exemple a différents problèmes:

• La fonction membre n'utilise aucun de ses privilèges, ni ne s'y réfère this. Il agit comme une fonction utilitaire gratuite qui est simplement au mauvais endroit.
• Votre classe n'essaie de présenter aucune sorte d' abstraction . Dans cette optique, il n'essaie pas d' encapsuler ses entrailles, ni de maintenir aucun invariant . C'est un sac stupide d'éléments arbitraires.

En résumé: oui, cet exemple est mauvais. Mais pas parce que la fonction membre obtient des objets membres passés.

Concrètement, cela est une mauvaise idée pour plusieurs bonnes raisons, mais je ne suis pas sûr que tous font partie intégrante de votre question:

• Avoir des variables de classe (des variables réelles, pas des constantes) est quelque chose à éviter dans la mesure du possible, et devrait déclencher une réflexion sérieuse sur la nécessité absolue.
• Laisser l'accès en écriture à ces variables de classe complètement ouvert à tout le monde est presque universellement mauvais.
• Votre méthode de classe finit par être sans rapport avec votre classe. En réalité, c'est une fonction qui ajoute les valeurs d'un tableau aux valeurs d'un autre tableau. Ce type de fonction devrait être une fonction dans un langage qui a des fonctions, et devrait être une méthode statique d'une "fausse classe" (c'est-à-dire une collection de fonctions sans données ni comportement d'objet) dans des langages qui n'ont pas de fonctions.
• Sinon, supprimez ces paramètres et transformez-les en une véritable méthode de classe, mais ce serait toujours mauvais pour les raisons mentionnées précédemment.
• Pourquoi des tableaux int? vector<int>vous faciliterait la vie.
• Si cet exemple est vraiment représentatif de votre conception, envisagez de placer vos données dans des objets et non dans des classes et également d'implémenter des constructeurs pour ces objets d'une manière qui ne nécessite pas de modifier la valeur des données d'objet après la création.

Il s'agit d'une approche classique «C avec classes» du C ++. En réalité, ce n'est pas ce qu'un programmeur C ++ expérimenté écrirait. D'une part, l'utilisation de tableaux C bruts est à peu près universellement déconseillée, sauf si vous implémentez une bibliothèque de conteneurs.

Quelque chose comme ça serait plus approprié:

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