¿Cómo refactorizar el código a algún código común?

16

Background

Estoy trabajando en un proyecto de C # en curso. No soy un programador de C #, principalmente un programador de C ++. Así que me asignaron tareas básicamente fáciles y de refactorización.

El código es un desastre. Es un gran proyecto. Como nuestro cliente exigía lanzamientos frecuentes con nuevas características y correcciones de errores, todos los demás desarrolladores se vieron obligados a adoptar el enfoque de fuerza bruta mientras codificaban. El código es altamente inalcanzable y todos los demás desarrolladores están de acuerdo con él.

No estoy aquí para debatir si lo hicieron bien. ¡Mientras estoy refactorizando, me pregunto si lo estoy haciendo de la manera correcta ya que mi código refactorizado parece complejo! Aquí está mi tarea como ejemplo simple.

Problema

Hay seis clases: A , B , C , D , E y F . Todas las clases tienen una función ExecJob() . Las seis implementaciones son muy similares. Básicamente, al principio se escribió A::ExecJob() . Luego se requirió una versión ligeramente diferente que se implementó en B::ExecJob() mediante la copia-pegado-modificación de A::ExecJob() . Cuando se requería otra versión ligeramente diferente, se escribió C::ExecJob() y así sucesivamente. Las seis implementaciones tienen un código común, luego algunas líneas de código diferentes, luego nuevamente un código común y así sucesivamente. Aquí hay un ejemplo simple de las implementaciones:

A::ExecJob()
{
    S1;
    S2;
    S3;
    S4;
    S5;
}

B::ExecJob()
{
    S1;
    S3;
    S4;
    S5;
}

C::ExecJob()
{
    S1;
    S3;
    S4;
}

Donde SN es un grupo de exactamente las mismas declaraciones.

Para hacerlos comunes, he creado otra clase y moví el código común en una función. Usar un parámetro para controlar qué grupo de sentencias debe ejecutarse:

Base::CommonTask(param)
{
    S1;
    if (param.s2) S2;
    S3;
    S4;
    if (param.s5) S5;
}

A::ExecJob() // A inherits Base
{
    param.s2 = true;
    param.s5 = true;
    CommonTask(param);
}

B::ExecJob() // B inherits Base
{
    param.s2 = false;
    param.s5 = true;
    CommonTask(param);
}

C::ExecJob() // C inherits Base
{
    param.s2 = false;
    param.s5 = false;
    CommonTask(param);
}

Tenga en cuenta que, este ejemplo solo emplea tres clases y declaraciones simplificadas. En la práctica, la función CommonTask() parece muy compleja con todos los parámetros de verificación y hay muchas más declaraciones. Además, en el código real, hay varias funciones de búsqueda de CommonTask() .

A pesar de que todas las implementaciones comparten código común y las funciones ExecJob() se ven más lindas, existen dos problemas que me molestan:

  • Para cualquier cambio en CommonTask() , se necesitan las seis características (y puede que haya más en el futuro) para ser probadas.
  • CommonTask() ya es complejo. Se volverá más complejo con el tiempo.

¿Lo estoy haciendo de la manera correcta?

    
pregunta Donotalo 24.11.2011 - 06:20

7 respuestas

14

¡Sí, estás absolutamente en el camino correcto!

En mi experiencia, noté que cuando las cosas son complicadas, los cambios ocurren en pequeños pasos. Lo que has hecho es el paso 1 en el proceso de evolución (o proceso de refactorización). Aquí están los pasos 2 y 3:

Paso 2

class Base {
  method ExecJob() {
    S1();
    S2();
    S3();
    S4();
    S5();
  }
  method S1() { //concrete implementation }
  method S3() { //concrete implementation }
  method S4() { //concrete implementation}
  abstract method S2();
  abstract method S5();
}

class A::Base {
  method S2() {//concrete implementation}
  method S5() {//concrete implementation}
}

class B::Base {
  method S2() { // empty implementation}
  method S5() {//concrete implementation}
}

class C::Base {
  method S2() { // empty implementation}
  method S5() { // empty implementation}
}

Este es el 'Patrón de diseño de plantillas' y está un paso adelante en el proceso de refactorización. Si la clase base cambia, las subclases (A, B, C) no necesitan verse afectadas. Puede agregar nuevas subclases con relativa facilidad. Sin embargo, desde la imagen de arriba se puede ver que La abstracción está rota. La necesidad de una 'implementación vacía' es un buen indicador; muestra que hay algo mal con tu abstracción. Podría haber sido una solución aceptable para el corto plazo, pero parece haber una mejor.

Paso 3

interface JobExecuter {
  void executeJob();
}
class A::JobExecuter {
  void executeJob(){
     helper = new Helper();
     helper->S1();
     helper->S2();
     helper->S3();
     helper->S4();
     helper->S5();
  }
}

class B::JobExecuter {
  void executeJob(){
     helper = new Helper();
     helper->S1();
     helper->S3();
     helper->S4();
     helper->S5();
  }
}

class C::JobExecuter {
  void executeJob(){
     helper = new Helper();
     helper->S1();
     helper->S3();
     helper->S4();
  }
}

class Base{
   void ExecJob(JobExecuter executer){
       executer->executeJob();
   }
}

class Helper{
    void S1(){//Implementation} 
    void S2(){//Implementation}
    void S3(){//Implementation}
    void S4(){//Implementation} 
    void S5(){//Implementation}
}

Este es el 'Patrón de diseño de estrategia' y parece ser un buen ajuste para su caso. Existen diferentes estrategias para ejecutar el trabajo y cada clase (A, B, C) lo implementa de manera diferente.

Estoy seguro de que hay un paso 4 o un paso 5 en este proceso o muchos enfoques de refactorización mucho mejores. Sin embargo, este le permitirá eliminar el código duplicado y asegurarse de que los cambios estén localizados.

    
respondido por el Guven 24.11.2011 - 07:08
7

En realidad estás haciendo lo correcto. Digo esto porque:

  1. Si necesita cambiar el código para una funcionalidad de tarea común, no necesita cambiarlo en las 6 clases que contendrían el código si no lo escribe en una clase común.
  2. Se reducirá la cantidad de líneas de código.
respondido por el prema 24.11.2011 - 06:33
3

Ves que este tipo de código se comparte mucho con el diseño dirigido por eventos (especialmente .NET). La forma más fácil de mantener es mantener su comportamiento compartido en partes tan pequeñas como sea posible.

Deje que el código de alto nivel reutilice un montón de pequeños métodos, deje el código de alto nivel fuera de la base compartida.

Tendrá una gran cantidad de placa de caldera en sus implementaciones de hoja / concreto. No te asustes, está bien. Todo ese código es directo, fácil de entender. Tendrá que reorganizarlo ocasionalmente cuando se rompan las cosas, pero será fácil de cambiar.

Verás muchos patrones en el código de alto nivel. A veces son reales, la mayoría de las veces no lo son. Las "configuraciones" de los cinco parámetros arriba parecen similares, pero no lo son. Son tres estrategias completamente diferentes.

También quiero tener en cuenta que puedes hacer todo esto con la composición y nunca preocuparte por la herencia. Tendrás menos acoplamiento.

    
respondido por el Tom Kerr 24.11.2011 - 07:45
3

Si yo fuera tú, probablemente agregaré 1 paso más al principio: un estudio basado en UML.

Refactorizar el código fusionar todas las partes comunes no siempre es el mejor movimiento, parece más una solución temporal que un buen enfoque.

Dibuja un esquema UML, mantén las cosas simples pero efectivas, recuerda algunos conceptos básicos sobre tu proyecto, como "¿qué se supone que debe hacer este software?" "¿Cuál es la mejor manera de mantener este software abstracto, modular, extensible, etc., etc.?" "¿Cómo puedo implementar la encapsulación de la mejor manera?"

Solo digo esto: no te preocupes por el código en este momento, solo tienes que preocuparte por la lógica, cuando tienes una lógica clara en mente, todo lo demás puede convertirse en una tarea muy fácil, al final, todo El tipo de problemas que enfrenta es simplemente causado por una mala lógica.

    
respondido por el Micro 24.11.2011 - 08:07
3

El primer paso, sin importar a dónde vaya, debe dividir el método aparentemente grande A::ExecJob en partes más pequeñas.

Por lo tanto, en lugar de

A::ExecJob()
{
    S1; // many lines of code
    S2; // many lines of code
    S3; // many lines of code
    S4; // many lines of code
    S5; // many lines of code
}

obtienes

A::ExecJob()
{
    S1();
    S2();
    S3();
    S4();
    S5();
}

A:S1()
{
   // many lines of code
}

A:S2()
{
   // many lines of code
}

A:S3()
{
   // many lines of code
}

A:S4()
{
   // many lines of code
}

A:S5()
{
   // many lines of code
}

De aquí en adelante, hay muchas maneras de ir. Mi opinión sobre esto: Convierta A en la clase base de su jerarquía de clase y ExecJob virtual y será más fácil crear B, C, ... sin mucho copiado y pegado: simplemente reemplace ExecJob (ahora de cinco líneas) con una versión modificada.

B::ExecJob()
{
    S1();
    S3();
    S4();
    S5();
}

¿Pero por qué tienen tantas clases? Tal vez pueda reemplazarlos a todos con una sola clase que tenga un constructor al que se le puede decir qué acciones son necesarias en ExecJob .

    
respondido por el user281377 24.11.2011 - 09:07
2

Estoy de acuerdo con las otras respuestas en que su enfoque está en la línea correcta, aunque no creo que la herencia sea la mejor manera de implementar un código común, prefiero la composición. De las preguntas frecuentes de C ++ que pueden explicarlo mucho mejor de lo que yo podría: enlace

    
respondido por el Ant 29.05.2014 - 18:36
1

Primero, debe asegurarse de que la herencia sea realmente la herramienta correcta aquí para el trabajo, solo porque necesita un lugar común para las funciones utilizadas por sus clases A a F no significa que una clase base común sea Lo correcto aquí: a veces, una clase de ayuda separada hace mejor el trabajo. Puede ser, puede no ser. Eso depende de tener una relación "is-a" entre A y F y su clase base común, imposible de decir de los nombres artificiales A-F. Aquí encuentra una publicación de blog que trata este tema.

Supongamos que usted decide que la clase base común es lo correcto en su caso. Entonces Lo segundo que haría es asegurarme de que los fragmentos de código S1 a S5 estén implementados en métodos separados de S1() a S5() de su clase base. Luego, las funciones "ExecJob" deberían verse así:

A::ExecJob()
{
    S1();
    S2();
    S3();
    S4();
    S5();
}

B::ExecJob()
{
    S1();
    S3();
    S4();
    S5();
}

C::ExecJob()
{
    S1();
    S3();
    S4();
}

Como se ve ahora, dado que S1 a S5 son solo llamadas de método, ya no hay bloqueos de código, la duplicación de código se ha eliminado casi por completo y ya no necesita ninguna comprobación de parámetros, evitando el problema de complejidad creciente podría obtener lo contrario.

Finalmente, pero solo como un tercer paso (!), puede pensar en combinar todos esos métodos ExecJob en una de su clase base, donde la ejecución de esas partes puede controlarse mediante parámetros, tal como lo sugirió. o utilizando el patrón de método de plantilla. Usted debe decidir si ese esfuerzo vale la pena en su caso, según el código real.

Pero IMHO, la técnica básica para dividir los métodos grandes en métodos pequeños es mucho, mucho más importante para evitar la duplicación de código que para aplicar patrones.

    
respondido por el Doc Brown 24.11.2011 - 08:28

Lea otras preguntas en las etiquetas