¿Es mejor tener 2 métodos con un significado claro, o solo 1 método de doble uso?

30

Para simplificar la interfaz, ¿es mejor simplemente no tener el getBalance()método? Pasar 0a la charge(float c);voluntad dará el mismo resultado:

public class Client {
    private float bal;
    float getBalance() { return bal; }
    float charge(float c) {
        bal -= c;
        return bal;
    }
}

Tal vez hacer una nota en javadoc? ¿O simplemente deja que el usuario de la clase descubra cómo obtener el saldo?

david
fuente
26
Supongo generosamente que este es un ejemplo, un código ilustrativo y que no está utilizando las matemáticas de punto flotante para almacenar valores monetarios. De lo contrario, tendría que predicar todo. :-)
corsiKa
1
@corsiKa nah. Eso es solo un ejemplo. Pero sí, hubiera usado float para representar dinero en una clase real ... Gracias por recordarme sobre los peligros de los números de coma flotante.
David
10
Algunos lenguajes distinguen entre mutadores y accesores con buenos resultados. ¡Sería muy molesto no poder obtener el equilibrio de una instancia a la que solo se puede acceder como una constante!
JDługosz

Respuestas:

90

Parece sugerir que la complejidad de una interfaz se mide por la cantidad de elementos que tiene (métodos, en este caso). Muchos argumentarían que tener que recordar que el chargemétodo se puede usar para devolver el equilibrio de un Clientagrega mucha más complejidad que tener el elemento adicional del getBalancemétodo. Hacer las cosas más explícitas es mucho más simple, especialmente hasta el punto en que no deja ambigüedad, independientemente del mayor número de elementos en la interfaz.

Además, las llamadas charge(0)violan el principio de menor asombro , también conocido como la métrica de WTF por minuto (de Clean Code, imagen a continuación), lo que dificulta a los nuevos miembros del equipo (o los actuales, después de un tiempo alejado del código) hasta entienden que la llamada se usa realmente para obtener el saldo. Piense en cómo reaccionarían otros lectores:

ingrese la descripción de la imagen aquí

Además, la firma del chargemétodo va en contra de las pautas de hacer una sola cosa y la separación de consulta de comando , porque hace que el objeto cambie su estado al tiempo que devuelve un nuevo valor.

En general, creo que la interfaz más simple en este caso sería:

public class Client {
  private float bal;
  float getBalance() { return bal; }
  void charge(float c) { bal -= c; }
}
MichelHenrich
fuente
25
El punto sobre la separación comando-consulta es extremadamente importante, en mi opinión. Imagina que eres un nuevo desarrollador en este proyecto. Al revisar el código, queda claro de inmediato que getBalance()devuelve algún valor y no modifica nada. Por otro lado, charge(0)parece que probablemente modifica algo ... ¿tal vez envía un evento PropertyChanged? ¿Y qué está devolviendo, el valor anterior o el nuevo valor? Ahora tiene que buscarlo en los documentos y desperdiciar la capacidad intelectual, lo que podría haberse evitado utilizando un método más claro.
Mage Xy
19
Además, usarlo charge(0)como la forma de obtener el equilibrio porque resulta que no tiene ningún efecto ahora lo conecta con este detalle de implementación; Puedo imaginar fácilmente un requisito futuro en el que el seguimiento de la cantidad de cargos se vuelve relevante, en ese momento tener su base de código llena de cargos que no son realmente cargos se convierte en un punto de dolor. Debe proporcionar elementos de interfaz que signifiquen las cosas que sus clientes deben hacer, no los que simplemente hacen las cosas que sus clientes deben hacer.
Ben
12
La advertencia de CQS no es necesariamente apropiada. Para un charge()método, si ese método es atómico en un sistema concurrente, puede ser apropiado devolver el valor nuevo o antiguo.
Dietrich Epp
3
Sus dos citas para CQRS me parecieron extremadamente poco convincentes. Por ejemplo, en general me gustan las ideas de Meyer, pero mirar el tipo de retorno de una función para saber si muta algo es arbitrario y no robusto. Muchas estructuras de datos concurrentes o inmutables requieren mutación + retornos. ¿Cómo agregarías una cadena sin violar CQRS? ¿Tienes alguna cita mejor?
user949300
66
Casi ningún código se ajusta a la Separación de consulta de comando. No es idiomático en ningún lenguaje popular orientado a objetos, y es violado por las API incorporadas de cada idioma que he usado. Por lo tanto, describirlo como una "mejor práctica" parece extraño; ¿Puedes nombrar incluso un proyecto de software que siga este patrón?
Mark Amery
28

En mi opinión, reemplazar getBalance()con charge(0)su aplicación no es una simplificación. Sí, son menos líneas, pero ofusca el significado del charge()método, lo que podría causar dolores de cabeza en el futuro cuando usted u otra persona necesite revisar este código.

Aunque podrían dar el mismo resultado, obtener el saldo de una cuenta no es lo mismo que un cargo de cero, por lo que probablemente sería mejor separar sus preocupaciones. Por ejemplo, si alguna vez necesita modificarcharge() para iniciar sesión cada vez que hay una transacción de cuenta, ahora tiene un problema y necesitaría separar la funcionalidad de todos modos.

Matt Dalzell
fuente
13

Es importante recordar que su código debe autodocumentarse. Cuando llamo charge(x), espero que me xcobren. La información sobre el equilibrio es secundaria. Además, puede que no sepa cómocharge() se implementa cuando lo llamo y definitivamente no sabré cómo se implementará mañana. Por ejemplo, considere esta posible actualización futura para charge():

float charge(float c) {
    lockDownUserAccountUntilChargeClears();
    bal -= c;
    Unlock();
    return bal;
}

De repente, usar charge()para obtener el equilibrio no se ve tan bien.

David dice reinstalar a Mónica
fuente
¡Sí! Un buen punto que chargees mucho más pesado.
Deduplicador
2

Usar charge(0);para obtener el saldo es una mala idea: un día, alguien podría agregar un código allí para registrar los cargos que se realizan sin darse cuenta del otro uso de la función, y luego, cada vez que alguien obtenga el saldo, se registrará como un cargo. (Hay formas de evitar esto, como una declaración condicional que dice algo como:

if (c > 0) {
    // make and log charge
}
return bal;

pero estos dependen de que el programador sepa implementarlos, lo cual no hará si no es inmediatamente obvio que son necesarios.

En resumen: no confíe en que sus usuarios o sus sucesores programadores charge(0);se den cuenta de que esa es la forma correcta de obtener el saldo, porque a menos que exista documentación que garantice que no se perderán, francamente esa parece ser la forma más aterradora de obtener el saldo posible.

Micheal Johnson
fuente
0

Sé que hay muchas respuestas, pero otra razón en contra charge(0)es que un simple error tipográfico charge(9)hará que el saldo de su cliente disminuya cada vez que desee obtener su saldo. Si tiene una buena prueba de unidad, podría mitigar ese riesgo, pero si no es diligente en cada llamada charge, podría tener este contratiempo.

Shaz
fuente
-2

Me gustaría mencionar un caso particular en el que tendría sentido tener menos métodos, más multipropósito: si hay mucho polimorfismo, es decir, muchas implementaciones de esta interfaz ; especialmente si esas implementaciones están en código desarrollado por separado que no puede actualizarse en sincronización (la interfaz está definida por una biblioteca).

En ese caso, simplificar el trabajo de escribir cada implementación es mucho más valioso que la claridad del uso de la misma, porque la primera evita errores de violación de contrato (los dos métodos son inconsistentes entre sí), mientras que la segunda solo perjudica la legibilidad, lo que puede ser recuperado por una función auxiliar o método de superclase que define getBalanceen términos decharge .

(Este es un patrón de diseño, para el cual no recuerdo un nombre específico: definir una interfaz compleja amigable para las personas que llaman en términos de una interfaz mínima amigable para el implementador. En Mac OS clásico, la interfaz mínima para operaciones de dibujo se llamaba "cuello de botella" pero este no parece ser un término popular).

Si este no es el caso (hay pocas implementaciones o exactamente una), entonces tiene sentido separar los métodos para mayor claridad y permitir la simple adición de comportamiento relevante para cargas distintas de cero charge().

Kevin Reid
fuente
1
De hecho, he tenido casos en los que se eligió la interfaz más mínima para facilitar las pruebas de cobertura completa. Pero esto no está necesariamente expuesto al usuario de la clase. Por ejemplo , insertar , agregar y eliminar pueden ser envoltorios simples de un reemplazo general que tiene todas las rutas de control bien estudiadas y probadas.
JDługosz
He visto tal API. El asignador Lua 5.1+ lo usa. Proporciona una función y, en función de los parámetros que pasa, decide si está tratando de asignar nueva memoria, desasignar memoria o reasignar memoria de la memoria anterior. Es doloroso escribir tales funciones. Prefiero que Lua te haya dado dos funciones, una para la asignación y otra para la desasignación.
Nicol Bolas
@NicolBolas: ¿Y ninguno para reasignación? De todos modos, ese tiene el "beneficio" adicional de ser casi el mismo que realloc, a veces, en algunos sistemas.
Deduplicador
@Deduplicator: la asignación frente a la reasignación es ... OK. No es bueno ponerlos en la misma función, pero no es tan malo como poner la desasignación en la misma. También es una distinción fácil de hacer.
Nicol Bolas
Diría que combinar la desasignación con la (re) asignación es un ejemplo especialmente malo de este tipo de interfaz. (Deallocation tiene un contrato muy diferente: no da como resultado un puntero válido.)
Kevin Reid