Funciones de una línea que se llaman solo una vez

120

Considere una función sin parámetros ( editar: no necesariamente) que realiza una sola línea de código y se llama solo una vez en el programa (aunque no es imposible que se vuelva a necesitar en el futuro).

Podría realizar una consulta, verificar algunos valores, hacer algo relacionado con expresiones regulares ... cualquier cosa oscura o "hacky".

La razón detrás de esto sería evitar evaluaciones difícilmente legibles:

if (getCondition()) {
    // do stuff
}

¿Dónde getCondition()está la función de una línea?

Mi pregunta es sencilla: ¿es una buena práctica? Me parece bien, pero no sé a largo plazo ...

vemv
fuente
99
A menos que su fragmento de código sea de un lenguaje OOP con un receptor implícito (p. Ej., Esto), ciertamente esto sería una mala práctica, ya que getCondition () probablemente depende del estado global ...
Ingo
2
¿Puede haber querido decir que getCondition () podría tener argumentos?
user606723
24
@Ingo: algunas cosas realmente tienen un estado global. La hora actual, los nombres de host, los números de puerto, etc. son todos válidos "Globals". El error de diseño está haciendo que un Global sea algo inherentemente no global.
James Anderson el
1
¿Por qué no solo en línea getCondition? Si es tan pequeño y se usa con poca frecuencia como usted dice, entonces darle un nombre no está logrando nada.
davidk01
12
davidk01: legibilidad del código.
wjl

Respuestas:

240

Depende de esa línea. Si la línea es legible y concisa por sí misma, la función puede no ser necesaria. Ejemplo simplista:

void printNewLine() {
  System.out.println();
}

OTOH, si la función le da un buen nombre a una línea de código que contiene, por ejemplo, una expresión compleja y difícil de leer, está perfectamente justificada (para mí). Ejemplo contribuido (dividido en varias líneas para facilitar la lectura aquí):

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}
Péter Török
fuente
99
+1. La palabra mágica aquí es "Código autodocumentado".
Konamiman
8
Un buen ejemplo de lo que el tío Bob llamaría "condicionales encapsulantes".
Anthony Pegram
12
@Aditya: Nada dice que taxPayersea ​​global en este escenario. Quizás esta clase es TaxReturn, y taxPayeres un atributo.
Adam Robinson
2
Además, le permite documentar la función de una manera que puede ser recogida, por ejemplo, por javadoc y ser públicamente visible.
2
@dallin, el Código Limpio de Bob Martin muestra muchos ejemplos de códigos de la vida real. Si tiene demasiadas funciones en una sola clase, ¿tal vez su clase es demasiado grande?
Péter Török
67

Sí, esto puede usarse para satisfacer las mejores prácticas. Por ejemplo, es mejor tener una función claramente nombrada que haga algún trabajo, incluso si es solo una línea de largo, que tener esa línea de código dentro de una función más grande y necesitar un comentario de una línea que explique lo que hace. Además, las líneas de código vecinas deben realizar tareas en el mismo nivel de abstracción. Un contraejemplo sería algo así como

startIgnition();
petrolFlag |= 0x006A;
engageChoke();

En este caso, definitivamente es mejor mover la línea media a una función con un nombre sensato.

Kilian Foth
fuente
15
Ese 0x006A es encantador: una constante bien conocida de combustible carbonizado con aditivos a, byc.
Codificador
2
Por cierto, estoy a favor del código de autodocumentación :) Creo que estoy de acuerdo en mantener un nivel de abstracción constante a través de bloques, pero no puedo explicar por qué. ¿Te importaría expandir eso?
vemv
3
Si solo dice eso petrolFlag |= 0x006A;sin ningún tipo de toma de decisiones, sería mejor simplemente decir petrolFlag |= A_B_C; sin una función adicional. Presumiblemente, engageChoke()solo debería llamarse si petrolFlagcumple con ciertos criterios y eso debería decir claramente 'Necesito una función aquí'. Solo una cuestión menor, esta respuesta es básicamente acertada de lo contrario :)
Tim Post
99
+1 para señalar que el código dentro de un método debe estar en el mismo nivel de abstracción. @vemv, es una buena práctica porque hace que el código sea más fácil de entender, evitando la necesidad de que tu mente salte hacia arriba y hacia abajo entre diferentes niveles de abstracción mientras lees el código. Atar los conmutadores de nivel de abstracción a llamadas / devoluciones de método (es decir, "saltos" estructurales) es una buena manera de hacer que el código sea más fluido y limpio.
Péter Török
21
No, es un valor completamente inventado. Pero el hecho de que se pregunte al respecto solo enfatiza el punto: si el código lo hubiera dicho mixLeadedGasoline(), ¡no tendría que hacerlo!
Kilian Foth
37

Creo que en muchos casos esa función es un buen estilo, pero puede considerar una variable booleana local como alternativa en los casos en que no necesite usar esta condición en otro lugar, por ejemplo:

bool someConditionSatisfied = [complex expression];

Esto le dará una pista al lector de código y evitará la introducción de una nueva función.

cybevnm
fuente
1
El bool es particularmente beneficioso si el nombre de la función de condición sería difícil o posiblemente engañoso (por ejemplo IsEngineReadyUnlessItIsOffOrBusyOrOutOfService).
dbkk
2
Experimenté este consejo como una mala idea: el bool y la condición distraen la atención del negocio principal de la función. Además, un estilo que prefiere las variables locales dificulta la refactorización.
Wolf
1
@Wolf OTOH, prefiero esto a una llamada de función para reducir la "profundidad de abstracción" del código. En mi opinión, saltar a una función es un cambio de contexto más grande que un par de líneas de código explícito y directo, especialmente si solo devuelve un valor booleano.
Kache
@Kache Creo que depende de si codifica orientado a objetos o no, en el caso de OOP, el uso de la función miembro mantiene el diseño mucho más flexible. Realmente depende del contexto ...
Wolf
23

Además de la respuesta de Peter , si es posible que esa condición deba actualizarse en algún momento en el futuro, al encapsularla de la forma en que sugiere que solo tendría un único punto de edición.

Siguiendo el ejemplo de Peter, si esto

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isFemale() 
        && (taxPayer.getNumberOfChildren() > 2 
        || (taxPayer.getAge() > 50 && taxPayer.getEmployer().isNonProfit()));
}

se convierte en esto

boolean isTaxPayerEligibleForTaxRefund() {
  return taxPayer.isMutant() 
        && (taxPayer.getNumberOfThumbs() > 2 
        || (taxPayer.getAge() > 123 && taxPayer.getEmployer().isXMan()));
}

Realiza una única edición y se actualiza universalmente. Mantenibilidad sabia, esto es una ventaja.

En cuanto al rendimiento, la mayoría de los compiladores de optimización eliminarán la llamada a la función y de todos modos alinearán el bloque de código pequeño. La optimización de algo como esto en realidad puede reducir el tamaño del bloque (eliminando las instrucciones necesarias para la llamada a la función, devolución, etc.), por lo que normalmente es seguro hacerlo incluso en condiciones que de otro modo podrían evitar la inserción.

Stephen
fuente
2
Ya conozco los beneficios de mantener un solo punto de edición, por eso la pregunta dice "... llamó una vez". Sin embargo, es genial saber acerca de esas optimizaciones del compilador, siempre pensé que seguían este tipo de instrucciones literalmente.
vemv
La división de un método para probar una condición simple tiende a implicar que se puede cambiar una condición cambiando el método. Tal implicación es útil cuando es verdad, pero puede ser peligrosa cuando no lo es. Por ejemplo, suponga que el código necesita dividir cosas en objetos livianos, objetos verdes pesados ​​y objetos pesados ​​no verdes. Los objetos tienen una propiedad rápida "parece verde" que es confiable para objetos pesados, pero puede devolver falsos positivos con objetos livianos; también tienen una función "measureSpectralResponse" que es lenta, pero funcionará de manera confiable para todos los objetos.
supercat
El hecho de que seemsGreenno sea confiable con objetos livianos será irrelevante si el código nunca se preocupa si los objetos livianos son verdes. Sin embargo, si la definición de "ligero" cambia, de modo que algunos objetos no verdes que devuelven verdadero seemsGreenno se informan como "ligero", entonces dicho cambio en la definición de "ligero" podría romper el código que prueba los objetos ser verde". En algunos casos, las pruebas de ecología y peso juntas en el código pueden hacer que la relación entre las pruebas sea más clara que si fueran métodos separados.
supercat
5

Además de la legibilidad (o como complemento), esto permite escribir funciones en el nivel adecuado de abstracción.

tylermac
fuente
1
Me temo que no entiendo lo que quieres decir ...
vemv
1
@vemv: Creo que quiere decir con "nivel adecuado de abstracción" que no terminas con un código que tiene diferentes niveles de abstracción mezclados (es decir, lo que Kilian Foth ya dijo). No desea que su código maneje detalles aparentemente insignificantes (por ejemplo, la formación de todos los enlaces en el combustible) cuando el código circundante está considerando una vista de más alto nivel de la situación en cuestión (por ejemplo, ejecutar un motor). El primero abarrota al segundo y hace que su código sea menos fácil de leer y mantener, ya que tendrá que considerar todos los niveles de abstracción a la vez en cualquier momento.
Egon
4

Depende. A veces es mejor encapsular una expresión en una función / método, incluso si es solo una línea. Si es complicado de leer o lo necesita en varios lugares, entonces lo considero una buena práctica. A largo plazo, es más fácil de mantener, ya que ha introducido un único punto de cambio y una mejor legibilidad .

Sin embargo, a veces es algo que no necesitas. Cuando la expresión sea fácil de leer de todos modos y / o solo aparezca en un lugar, no la envuelva.

Halcón
fuente
3

Creo que si solo tiene algunos, está bien, pero el problema surge cuando hay muchos en su código. Y cuando se ejecuta el compilador o el interpitor (según el idioma que utilice), va a esa función en la memoria. Digamos que tiene 3 de ellos, no creo que la computadora se dé cuenta, pero si comienza a tener cientos de esas pequeñas cosas, el sistema tiene que registrar funciones en la memoria que solo se llaman una vez y luego no se destruyen.

WojonsTech
fuente
De acuerdo con la respuesta de Stephen, esto puede no suceder siempre (aunque confiar ciegamente en la magia de los compiladores no puede ser bueno de todos modos)
vemv
1
Sí, debería aclararse dependiendo de muchos hechos. Si es un lenguaje integrado, el sistema caerá para las funciones de una sola línea cada vez, a menos que instale algo para la memoria caché que aún no funciona. Ahora, cuando se trata de compiladores, solo importa el día de los débiles y la ubicación de los planos si el compilador será tu amigo y eliminará tu pequeño desorden o si pensará que realmente lo necesitas. Recuerdo que si conoce la cantidad exacta de veces que un bucle siempre funcionará, algunas veces es mejor simplemente copiarlo y pegarlo muchas veces para luego hacer un bucle.
WojonsTech
+1 para la alineación de los planetas :) pero tu última oración me suena totalmente loca, ¿realmente haces eso?
vemv
Realmente depende La mayoría de las veces no, no lo hago, a menos que esté recibiendo la cantidad correcta de pago para verificar si acelera la incisión y otras cosas por el estilo. Pero en compiladores más antiguos era mejor copiarlo y pegarlo que dejar el compilador para resolverlo el 9/10.
WojonsTech
3

He hecho esto exactamente recientemente en una aplicación que he estado refactorizando, para hacer explícito el significado real del código sin comentarios:

protected void PaymentButton_Click(object sender, EventArgs e)
    Func<bool> HaveError = () => lblCreditCardError.Text == string.Empty && lblDisclaimer.Text == string.Empty;

    CheckInputs();

    if(HaveError())
        return;

    ...
}
joshperry
fuente
Por curiosidad, ¿qué idioma es ese?
vemv
55
@vemv: A mí me parece C #.
Scott Mitchell
También prefiero identificadores adicionales a los comentarios. Pero, ¿esto realmente difiere de la introducción de una variable local para ifabreviar? Este enfoque local (lambda) hace que la PaymentButton_Clickfunción (como un todo) sea más difícil de leer. En lblCreditCardErrorsu ejemplo, parece ser un miembro, por lo que también HaveErrores un predicado (privado) que es válido para el objeto. Tiendo a rechazar esto, pero no soy un programador de C #, así que me resisto.
Wolf
@ Wolf Hola hombre, sí. Escribí esto hace bastante tiempo :) Definitivamente hago las cosas de manera muy diferente hoy en día. De hecho, mirar el contenido de la etiqueta para ver si hubo un error me da escalofríos ... ¿Por qué no devolví un bool de CheckInputs()???
joshperry
0

Mover esa línea a un método bien nombrado hace que su código sea más fácil de leer. Muchos otros ya lo han mencionado ("código autodocumentado"). La otra ventaja de pasarlo a un método es que facilita la prueba unitaria. Cuando se aísla en su propio método y se prueba la unidad, puede estar seguro de que si se encuentra un error, no estará en este método.

Andrés
fuente
0

Ya hay muchas buenas respuestas, pero hay un caso especial que vale la pena mencionar .

Si su declaración de una línea necesita un comentario , y puede identificar claramente (lo que significa: nombre) su propósito, considere extraer una función mientras mejora el comentario en el documento API. De esta forma, hace que la llamada a la función sea más rápida y fácil de entender.

Curiosamente, se puede hacer lo mismo si actualmente no hay nada que hacer, sino un comentario que recuerde las expansiones necesarias (en un futuro muy cercano 1) ), así que esto,

def sophisticatedHello():
    # todo set up
    say("hello")
    # todo tear down

bien podría cambiar a esto

def sophisticatedHello():
    setUp()
    say("hello")
    tearDown()

1) debes estar realmente seguro de esto (ver el principio de YAGNI )

Lobo
fuente
0

Si el lenguaje lo admite, generalmente uso funciones anónimas etiquetadas para lograr esto.

someCondition = lambda p: True if [complicated expression involving p] else False
#I explicitly write the function with a ternary to make it clear this is a a predicate
if (someCondition(p)):
    #do stuff...

En mi humilde opinión, este es un buen compromiso porque le brinda el beneficio de legibilidad de no tener la expresión complicada que satura la ifcondición mientras evita saturar el espacio de nombres global / paquete con pequeñas etiquetas desechables. Tiene el beneficio adicional de que la función "definición" está justo donde se está utilizando, lo que facilita la modificación y lectura de la definición.

No tiene que ser solo funciones predicadas. También me gusta encapsular la placa de caldera repetida en funciones pequeñas como esta (funciona particularmente bien para la generación de listas pitónicas sin saturar la sintaxis de paréntesis). Por ejemplo, el siguiente ejemplo demasiado simplificado cuando se trabaja con PIL en python

#goal - I have a list of PIL Image objects and I want them all as grayscale (uint8) numpy arrays
im_2_arr = lambda im: array(im.convert('L')) 
arr_list = [im_2_arr(image) for image in image_list]
crasico
fuente
¿Por qué "lambda p: Verdadero si [expresión complicada con p] sino Falso" en lugar de "lambda p: [expresión complicada con p]"? 8-)
Hans-Peter Störr
@hstoerr está en el comentario justo debajo de esa línea. Me gusta hacer saber explícitamente que someCondition es un predicado. Si bien es estrictamente innecesario, escribo muchos guiones científicos leídos por personas que no codifican tanto, personalmente creo que es más apropiado tener la terquedad adicional en lugar de confundir a mis colegas porque no saben eso [] == Falseo alguna otra cosa. equivalencia pitónica similar que no es "siempre" intuitiva. Básicamente es una forma de señalar que someCondition es, de hecho, un predicado.
crasic
Solo para aclarar mi error obvio, [] != Falsepero []es Falso como cuando lo lanzan a un bool
crasic
@crasic: cuando no espero que mis lectores sepan que [] se evalúa como False, prefiero hacerlo len([complicated expression producing a list]) == 0, en lugar de usar lo True if [blah] else Falseque aún requiere que el lector sepa que [] se evalúa como False.
Lie Ryan