¿Es una mala idea hacer que un método de clase se pase las variables de clase?

14

Esto es lo que quiero decir:

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

add ya puede acceder a todas las variables que necesita, ya que es un método de clase, ¿es una mala idea? ¿Hay razones por las que debería o no debería hacer esto?

    
pregunta swimingduck 09.12.2018 - 18:10
fuente

7 respuestas

33

Hay muchas cosas con la clase que haría de manera diferente, pero para responder la pregunta directa, mi respuesta sería

sí, es una mala idea

Mi principal razón para esto es que no tienes control sobre lo que se pasa a la función add . Seguro que esperas que sea uno de los arreglos miembros, pero ¿qué sucede si alguien pasa en un arreglo diferente que tiene un tamaño menor que 100, o si pasas en una longitud mayor que 100?

Lo que sucede es que se ha creado la posibilidad de un desbordamiento del búfer. Y eso es algo malo para todos.

Para responder algunas preguntas obvias más (para mí):

  1. Estás mezclando arreglos de estilo C con C ++. No soy un gurú de C ++, pero lo hago. Sepa que C ++ tiene formas mejores (más seguras) de manejar matrices

  2. Si el La clase ya tiene las variables miembro, ¿por qué necesitas pasarlas? ¿en? Esto es más una cuestión arquitectónica.

Otras personas con más experiencia en C ++ (dejé de usarlo hace 10 o 15 años) pueden tener formas más elocuentes de explicar los problemas y probablemente también presenten más problemas.     

respondido por el Peter M 09.12.2018 - 18:55
fuente
48

Llamar a un método de clase con algunas variables de clase no es necesariamente malo. Pero hacerlo desde fuera de la clase es una muy mala idea y sugiere un error fundamental en su diseño de OO, es decir, la ausencia de encapsulation :

  • Cualquier código que use su clase debería saber que len es la longitud de la matriz y usarla de manera consistente. Esto va en contra del principio del menor conocimiento . Dicha dependencia de los detalles internos de la clase es extremadamente propensa a errores y riesgosa.
  • Esto haría que la evolución de la clase sea muy difícil (por ejemplo, si un día desea cambiar los arreglos heredados y len a un std::vector<int> más moderno), ya que también es necesario cambiar todos los código utilizando su clase.
  • Cualquier parte del código podría causar estragos en sus objetos MyClass al corromper algunas variables públicas sin respetar las reglas (llamadas invariantes de clase )
  • Finalmente, el método es en realidad independiente de la clase, ya que funciona solo con los parámetros y no depende de ningún otro elemento de clase. Este tipo de método podría ser una función independiente fuera de la clase. O al menos un método estático.

Debes refactorizar tu código y:

  • haga que sus variables de clase private o protected , a menos que haya una buena razón para no hacerlo.
  • diseñe sus métodos públicos como acciones que se realizarán en la clase (por ejemplo, object.action(additional parameters for the action) ).
  • Si después de esta refactorización, aún tendrías algunos métodos de clase que deben llamarse con variables de clase, hazlos protegidos o privados después de haber verificado que son funciones de utilidad que admiten los métodos públicos.
respondido por el Christophe 09.12.2018 - 19:54
fuente
7

Cuando la intención de este diseño es que desea poder reutilizar este método para datos que no provienen de esta instancia de clase, es posible que desee declararlo como un método static .

Un método estático no pertenece a ninguna instancia particular de una clase. Es más bien un método de la clase en sí. Debido a que los métodos estáticos no se ejecutan en el contexto de una instancia particular de la clase, solo pueden acceder a las variables miembro que también se declaran como estáticas. Teniendo en cuenta que su método add no hace referencia a ninguna de las variables miembro declaradas en MyClass , es un buen candidato para ser declarado como estático.

Sin embargo, los problemas de seguridad mencionados por las otras respuestas son válidos: no está comprobando si ambas matrices tienen al menos len de largo. Si desea escribir C ++ moderno y robusto, debe evitar el uso de matrices. Use la clase std::vector en su lugar siempre que sea posible. Contrariamente a las matrices regulares, los vectores conocen su propio tamaño. Por lo tanto, no necesita realizar un seguimiento de su tamaño. La mayoría de sus métodos (¡no todos!) También realizan una comprobación de límites automática, lo que garantiza que obtendrá una excepción cuando lea o escriba más allá del final. El acceso regular a la matriz no hace una verificación de límites, lo que resulta en un segfault en el mejor de los casos y podría causar una vulnerabilidad de desbordamiento de búfer explotable peor.

    
respondido por el Philipp 10.12.2018 - 12:58
fuente
1

Un método en una clase idealmente manipula datos encapsulados dentro de la clase. En su ejemplo, no hay razón para que el método add tenga parámetros, simplemente use las propiedades de la clase.

    
respondido por el Rik D 09.12.2018 - 18:55
fuente
0

Pasar (parte de) un objeto a una función miembro está bien. Al implementar sus funciones, siempre debe conocer los posibles alias y sus consecuencias.

Por supuesto, el contrato puede dejar algunos tipos de alias sin definir, incluso si el idioma lo admite.

Ejemplos destacados:

  • Autoasignación. Esto debería ser un, posiblemente caro, no op. No peses el caso común para ello.
    Por otro lado, la auto-asignación de movimientos, mientras que no debería explotar en tu cara, no tiene por qué ser una no operación.
  • Insertando una copia de un elemento en un contenedor en el contenedor. Algo como v.push_back(v[2]); .
  • std::copy() asume que el destino no comienza dentro de la fuente.

Pero tu ejemplo tiene diferentes problemas:

  • La función miembro no usa ninguno de sus privilegios, ni se refiere a this . Actúa como una función de utilidad gratuita que está simplemente en el lugar equivocado.
  • Su clase no intenta presentar ningún tipo de abstracción . En línea con eso, no intenta encapsular sus entrañas, ni mantener invariantes . Es una bolsa de elementos arbitrarios.

En resumen: Sí, este ejemplo es malo. Pero no porque la función miembro se pasa objetos-miembro.

    
respondido por el Deduplicator 10.12.2018 - 13:14
fuente
0

Específicamente, hacer esto es una mala idea por varias buenas razones, pero no estoy seguro de que todas sean parte integral de tu pregunta:

  • Tener variables de clase (variables reales, no constantes) es algo que se debe evitar si es posible, y debería provocar una reflexión seria sobre si es absolutamente necesario.
  • Dejar el acceso de escritura a estas variables de clase completamente abierto para todos es casi universalmente malo.
  • Tu método de clase termina sin estar relacionado con tu clase. Lo que realmente es es una función que agrega los valores de una matriz a los valores de otra matriz. Este tipo de función debe ser una función en un lenguaje que tiene funciones, y debe ser un método estático de una "clase falsa" (es decir, una colección de funciones sin datos o comportamiento de objetos) en idiomas que no tienen funciones.
  • Como alternativa, elimine estos parámetros y conviértalos en un método de clase real, pero aún sería malo por las razones mencionadas anteriormente.
  • ¿Por qué int arrays? vector<int> haría tu vida más fácil.
  • Si este ejemplo es realmente representativo de su diseño, considere colocar sus datos en objetos y no en clases y también implementar constructores para estos objetos de una manera que no requiera cambiar el valor de los datos del objeto después de la creación.
respondido por el Kafein 10.12.2018 - 14:17
fuente
0

Este es un enfoque clásico de "C con clases" para C ++. En realidad, esto no es lo que escribiría cualquier programador experimentado de C ++. Por un lado, el uso de matrices C en bruto está bastante desaconsejado universalmente, a menos que esté implementando una biblioteca de contenedores.

Algo como esto sería más apropiado:

// 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
    }
}
    
respondido por el Alexander 11.12.2018 - 06:08
fuente

Lea otras preguntas en las etiquetas