Come rifattorizzare il codice su un codice comune?

Sì, sei assolutamente sulla strada giusta!

Nella mia esperienza, ho notato che quando le cose sono complicate, i cambiamenti avvengono a piccoli passi. Quello che hai fatto è il passaggio 1 nel processo di evoluzione (o processo di refactoring). Ecco il passaggio 2 e il passaggio 3:

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

Questo è il 'Template Design Pattern' ed è un passo avanti nel processo di refactoring. Se la classe base cambia, le sottoclassi (A, B, C) non devono essere influenzate. È possibile aggiungere nuove sottoclassi in modo relativamente semplice. Tuttavia, subito dopo l'immagine sopra puoi vederlo l'astrazione è rotta. La necessità di "implementazione vuota" è un buon indicatore; mostra che c'è qualcosa di sbagliato nella tua astrazione. Potrebbe essere stata una soluzione accettabile a breve termine, ma sembra esserci una soluzione migliore.

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

Questo è il "Modello di design della strategia" e sembra adatto al tuo caso. Esistono diverse strategie per eseguire il lavoro e ogni classe (A, B, C) la implementa in modo diverso.

Sono sicuro che ci sia un passaggio 4 o un passaggio 5 in questo processo o approcci di refactoring molto migliori. Tuttavia, questo ti permetterà di eliminare il codice duplicato e assicurarti che le modifiche siano localizzate.

In realtà stai facendo la cosa giusta. Dico questo perché:

1. Se è necessario modificare il codice per una funzionalità di attività comune, non è necessario cambiarlo in tutte e 6 le classi che conterranno il codice se non lo si scrive in una classe comune.
2. Il numero di righe di codice verrà diminuito.

Si vede questo tipo di codice che condivide molto con il design basato sugli eventi (specialmente .NET). Il modo più gestibile è di mantenere il tuo comportamento condiviso nel minor numero di pezzi possibile.

Lascia che il codice di alto livello riutilizzi una serie di piccoli metodi, lascia il codice di alto livello fuori dalla base condivisa.

Avrai molta piastra della caldaia nella tua foglia / applicazioni in calcestruzzo. Non farti prendere dal panico, va bene. Tutto quel codice è diretto, facile da capire. Dovrai riorganizzarlo di tanto in tanto quando le cose si rompono, ma sarà facile cambiarle.

Vedrai molti schemi nel codice di alto livello. A volte sono reali, il più delle volte non lo sono. Le "configurazioni" dei cinque parametri lassù sembrano simili, ma non lo sono. Sono tre strategie completamente diverse.

Inoltre, tieni presente che puoi fare tutto questo con la composizione e non preoccuparti mai dell'ereditarietà. Avrai meno accoppiamenti.

Se fossi in te probabilmente aggiungerò un altro passo all'inizio: uno studio basato su UML.

Il refactoring del codice che unisce insieme tutte le parti comuni non è sempre la mossa migliore, sembra più una soluzione temporanea che un buon approccio.

Disegna uno schema UML, mantiene le cose semplici ma efficaci, tieni a mente alcuni concetti di base sul tuo progetto come "cosa dovrebbe fare questo software?" "qual è il modo migliore per mantenere questo software astratto, modulare, estensibile, ... ecc. ecc.?" "come posso implementare l'incapsulamento al suo meglio?"

Sto solo dicendo questo: non ti interessa il codice adesso, devi solo preoccuparti della logica, quando hai una chiara logica in mente tutto il resto può diventare un compito davvero facile, alla fine tutto questo tipo di problemi che stai affrontando è semplicemente causato da una cattiva logica.

Il primo passo, indipendentemente da dove sta andando, dovrebbe essere la rottura del metodo apparentemente grande A::ExecJob in pezzi più piccoli.

Pertanto, anziché

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
}

ottieni

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
}

Da qui in poi, ci sono molti modi possibili per andare. La mia opinione su di esso: Rendi una classe base della tua classe gerarchia ed ExecJob virtuale e diventa facile creare B, C, ... senza troppo copia-incolla: basta sostituire ExecJob (ora un cinque-liner) con una versione modificata.

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

Ma perché ci sono così tante classi? Forse puoi sostituirli tutti con una singola classe che ha un costruttore che può dire quali azioni sono necessarie in ExecJob .

Sono d'accordo con le altre risposte sul fatto che il tuo approccio è sulla buona strada anche se non penso che l'ereditarietà sia il modo migliore per implementare il codice comune - preferisco la composizione. Dalle FAQ del C ++ che possono spiegarlo molto meglio di quanto io abbia mai potuto: link

In primo luogo, dovresti assicurarti che l'ereditarietà sia davvero lo strumento giusto qui per il lavoro - solo perché hai bisogno di un posto comune per le funzioni usate dalle tue classi A a F non significa che una classe base comune sia la cosa giusta qui - a volte una classe di aiuto separata fa meglio il lavoro. Potrebbe essere, potrebbe non esserlo. Ciò dipende dall'avere una relazione "is-a" tra A e F e la tua classe base comune, impossibile da dire con nomi artificiali A-F. Qui trovi un post sul blog relativo a questo argomento.

Supponiamo che tu decida che la classe base comune è la cosa giusta nel tuo caso. Poi la seconda cosa che farei è assicurarsi che i frammenti di codice da S1 a S5 vengano implementati ciascuno in metodi separati da S1() a S5() della classe base. Successivamente le funzioni di "ExecJob" dovrebbero apparire così:

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

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

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

Come si vede ora, dal momento che S1 a S5 sono solo chiamate di metodo, nessun codice blocca più, la duplicazione del codice è stata quasi completamente rimossa e non è più necessario alcun controllo dei parametri, evitando il problema di aumentare la complessità potresti ottenere altrimenti.

Infine, ma solo come terzo passaggio (!), potresti pensare di combinare tutti quei metodi ExecJob in una delle tue classi base, dove l'esecuzione di quelle parti può essere controllata da parametri, proprio come tu l'hai suggerito, o utilizzando il modello di metodo del modello. Devi decidere tu stesso se vale la pena nel tuo caso, in base al codice reale.

Ma IMHO la tecnica di base per abbattere i grandi metodi in piccoli metodi è molto, molto più importante per evitare la duplicazione del codice rispetto all'applicazione dei pattern.