Jak refaktoryzować kod do jakiegoś wspólnego kodu?

tło

Pracuję nad trwającym projektem C #. Nie jestem programistą C #, przede wszystkim programistą C ++. Więc przydzielono mi w zasadzie łatwe i refaktoryzujące zadania.

Kod to bałagan. To ogromny projekt. Ponieważ nasz klient domagał się częstych wydań z nowymi funkcjami i poprawkami błędów, wszyscy inni programiści byli zmuszeni stosować brutalną siłę podczas kodowania. Kod jest wysoce niemożliwy do utrzymania i wszyscy inni programiści się z nim zgadzają.

Nie jestem tutaj, by debatować, czy zrobili to dobrze. Podczas refaktoryzacji zastanawiam się, czy robię to we właściwy sposób, ponieważ mój refaktoryzowany kod wydaje się skomplikowany! Oto moje zadanie jako prosty przykład.

Problem

Istnieje sześć klas: A, B, C, D, Ei F. Wszystkie klasy mają funkcję ExecJob(). Wszystkie sześć implementacji jest bardzo podobnych. Zasadniczo na początku A::ExecJob()zostało napisane. Następnie wymagana była nieco inna wersja, która została zaimplementowana B::ExecJob()przez modyfikację kopiuj-wklej- A::ExecJob(). Kiedy wymagana C::ExecJob()była inna, nieco inna wersja, została napisana i tak dalej. Wszystkie sześć implementacji ma jakiś wspólny kod, następnie kilka różnych wierszy kodu, a następnie znowu wspólny kod i tak dalej. Oto prosty przykład realizacji:

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

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

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

Gdzie SNjest grupa dokładnie takich samych stwierdzeń.

Aby były wspólne, stworzyłem inną klasę i przeniosłem wspólny kod w funkcji. Użycie parametru do kontroli, która grupa instrukcji powinna zostać wykonana:

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

Zauważ, że w tym przykładzie zastosowano tylko trzy klasy i uproszczone instrukcje. W praktyce CommonTask()funkcja wygląda na bardzo złożoną przy wszystkich tych kontrolach parametrów i jest o wiele więcej instrukcji. Ponadto w prawdziwym kodzie znajduje się kilka CommonTask()funkcji podglądających.

Chociaż wszystkie implementacje współużytkują wspólny kod, a ExecJob()funkcje wyglądają ładniej, istnieją dwa problemy, które mnie niepokoją:

• W przypadku każdej zmiany CommonTask()należy przetestować wszystkie sześć (i być może w przyszłości) funkcji.
• CommonTask()jest już złożony. Z czasem stanie się bardziej złożony.

Czy robię to we właściwy sposób?

Tak, jesteś absolutnie na dobrej drodze!

Z mojego doświadczenia zauważyłem, że gdy sprawy są skomplikowane, zmiany następują małymi krokami. To, co zrobiłeś, to krok 1 w procesie ewolucji (lub procesie refaktoryzacji). Oto krok 2 i krok 3:

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

Jest to „wzorzec projektowania szablonu” i jest o krok do przodu w procesie refaktoryzacji. Jeśli klasa podstawowa ulegnie zmianie, nie ma to wpływu na podklasy (A, B, C). Możesz stosunkowo łatwo dodawać nowe podklasy. Jednak od razu na powyższym obrazku widać, że abstrakcja jest zepsuta. Potrzeba „pustego wdrożenia” jest dobrym wskaźnikiem; pokazuje, że coś jest nie tak z twoją abstrakcją. To może być do przyjęcia rozwiązanie krótkoterminowe, ale wydaje się, że jest lepsze.

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

Jest to „wzorzec projektowania strategii” i wydaje się, że pasuje do twojego przypadku. Istnieją różne strategie wykonywania zadania i każda klasa (A, B, C) realizuje je inaczej.

Jestem pewien, że w tym procesie jest krok 4 lub krok 5 lub o wiele lepsze metody refaktoryzacji. Ten pozwoli jednak wyeliminować zduplikowany kod i upewnić się, że zmiany są zlokalizowane.

Właściwie postępujesz właściwie. Mówię to, ponieważ:

1. Jeśli musisz zmienić kod dla wspólnej funkcji zadania, nie musisz go zmieniać we wszystkich 6 klasach, które zawierałyby kod, jeśli nie napiszesz go we wspólnej klasie.
2. Liczba wierszy kodu zostanie zmniejszona.

Ten typ kodu często współdzieli się z projektami sterowanymi zdarzeniami (szczególnie .NET). Najłatwiejszym do utrzymania sposobem jest utrzymanie wspólnego zachowania w jak najmniejszych częściach.

Pozwól kodowi wysokiego poziomu ponownie wykorzystać kilka małych metod, zostaw kod wysokiego poziomu poza współużytkowanej bazie.

Będziesz miał dużo płyty kotłowej we wdrożeniach liści / betonu. Nie panikuj, w porządku. Cały ten kod jest bezpośredni, łatwy do zrozumienia. Będziesz musiał od czasu do czasu zmieniać układ, gdy coś się zepsuje, ale łatwo będzie to zmienić.

Zobaczysz wiele wzorów w kodzie wysokiego poziomu. Czasami są prawdziwe, przez większość czasu nie są. „Konfiguracje” pięciu parametrów tam na górze wyglądają podobnie, ale nie są. Są to trzy zupełnie różne strategie.

Pamiętaj też, że możesz to wszystko zrobić z kompozycją i nigdy nie martwić się o dziedziczenie. Będziesz miał mniej sprzężenia.

Gdybym był tobą, prawdopodobnie dodam jeszcze jeden krok na początku: badanie oparte na UML.

Refaktoryzacja kodu łączącego wszystkie wspólne części razem nie zawsze jest najlepszym ruchem, brzmi bardziej jak rozwiązanie tymczasowe niż dobre podejście.

Rysuje schemat UML, spraw, aby rzeczy były proste, ale skuteczne, pamiętaj o kilku podstawowych koncepcjach dotyczących twojego projektu, takich jak „co ma robić to oprogramowanie?” „jaki jest najlepszy sposób, aby ten program był abstrakcyjny, modułowy, rozszerzalny, itp. itd.?” „jak mogę wdrożyć enkapsulację w najlepszym wydaniu?”

Mówię tylko: nie przejmuj się teraz kodem, musisz tylko dbać o logikę, kiedy masz jasną logikę, cała reszta może stać się naprawdę łatwym zadaniem, w końcu wszystko tego rodzaju problemy, z którymi się borykasz, są spowodowane jedynie złą logiką.

Pierwszym krokiem, bez względu na to, dokąd to zmierza, powinno być rozbicie pozornie dużej metody A::ExecJobna mniejsze części.

Dlatego zamiast

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
}

dostajesz

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
}

Odtąd istnieje wiele możliwych sposobów. Moje zdanie: uczyń A podstawową klasą hierarchii klas i ExecJob wirtualną, a tworzenie B, C, ... stanie się łatwe bez zbytniego kopiowania i wklejania - wystarczy zastąpić ExecJob (teraz pięcioliniowy) zmodyfikowanym wersja.

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

Ale po co w ogóle tyle zajęć? Być może możesz je wszystkie zastąpić jedną klasą, która ma konstruktor, który może powiedzieć, jakie działania są konieczne ExecJob.

Zgadzam się z innymi odpowiedziami, że twoje podejście jest prawidłowe, chociaż nie sądzę, że dziedziczenie jest najlepszym sposobem na wdrożenie wspólnego kodu - wolę kompozycję. Z C ++ FAQ, które mogą wyjaśnić to znacznie lepiej niż kiedykolwiek: http://www.parashift.com/c++-faq/priv-inherit-vs-compos.html

Po pierwsze, należy upewnić się, że dziedziczenie jest naprawdę właściwym narzędziem tu do pracy - tylko dlatego, że trzeba wspólne miejsce dla funkcji używanych przez swoich klas A, aby Fnie znaczy, że wspólna klasa bazowa jest słuszne tutaj - czasami oddzielny pomocnik klasa lepiej sobie radzi. Może być, może nie być. To zależy od posiadania relacji „od-do” między A do F a twoją wspólną klasą podstawową, czego nie można powiedzieć na podstawie sztucznych nazw AF. Tutaj znajduje się post na blogu dotyczący tego tematu.

Załóżmy, że zdecydujesz, że wspólna klasa podstawowa jest właściwa w twoim przypadku. Następnie drugą rzeczą, którą chciałbym zrobić, to upewnić się, że każdy fragment kodu od S1 do S5 jest zaimplementowany w osobnych metodach S1()dla S5()klasy podstawowej. Następnie funkcje „ExecJob” powinny wyglądać następująco:

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

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

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

Jak widać teraz, ponieważ S1 do S5 to tylko wywołania metod, nie ma już bloków kodu, duplikacja kodu została prawie całkowicie usunięta i nie trzeba już sprawdzać parametrów, unikając problemu zwiększonej złożoności, którą możesz uzyskać Inaczej.

Wreszcie, ale tylko jako trzeci krok (!), Możesz pomyśleć o połączeniu wszystkich metod ExecJob w jednej z klasy bazowej, gdzie wykonywanie tych części może być kontrolowane przez parametry, tak jak to sugerowałeś, lub za pomocą wzorzec metody szablonu. Musisz sam zdecydować, czy jest to warte wysiłku w twoim przypadku, na podstawie prawdziwego kodu.

Ale IMHO podstawowa technika dzielenia dużych metod na małe metody jest o wiele, o wiele ważniejsza dla uniknięcia powielania kodu niż stosowanie wzorców.