Bagaimana cara memperbaiki kode ke beberapa kode umum?

Latar Belakang

Saya sedang mengerjakan proyek C # yang sedang berjalan. Saya bukan programmer C #, terutama programmer C ++. Jadi saya ditugaskan pada dasarnya tugas yang mudah dan refactoring.

Kode ini berantakan. Ini proyek besar. Karena pelanggan kami sering menuntut rilis dengan fitur-fitur baru dan perbaikan bug, semua pengembang lainnya dipaksa untuk mengambil pendekatan brute force saat coding. Kode ini sangat tidak dapat dipelihara dan semua pengembang lain setuju dengannya.

Saya di sini bukan untuk berdebat apakah mereka melakukannya dengan benar. Ketika saya refactoring, saya bertanya-tanya apakah saya melakukannya dengan cara yang benar karena kode refactored saya kelihatannya kompleks! Inilah tugas saya sebagai contoh sederhana.

Masalah

Ada enam kelas: A, B, C, D, Edan F. Semua kelas memiliki fungsi ExecJob(). Keenam implementasi sangat mirip. Pada dasarnya, pada mulanya A::ExecJob()ditulis. Kemudian diperlukan versi yang sedikit berbeda yang diterapkan B::ExecJob()oleh copy-paste-modifikasi A::ExecJob(). Ketika versi lain yang sedikit berbeda diperlukan, C::ExecJob()ditulis dan sebagainya. Keenam implementasi memiliki beberapa kode umum, kemudian beberapa baris kode yang berbeda, lalu beberapa kode umum dan seterusnya. Berikut adalah contoh sederhana dari implementasinya:

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

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

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

Di mana SNsekelompok pernyataan yang sama persis.

Untuk menjadikannya umum, saya telah membuat kelas lain dan memindahkan kode umum dalam suatu fungsi. Menggunakan parameter untuk mengontrol grup pernyataan mana yang harus dieksekusi:

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

Perhatikan bahwa, contoh ini hanya menggunakan tiga kelas dan pernyataan yang terlalu disederhanakan. Dalam praktiknya, CommonTask()fungsinya terlihat sangat kompleks dengan semua pengecekan parameter dan masih banyak lagi pernyataan. Juga, dalam kode nyata, ada beberapa CommonTask()fungsi yang tampak.

Meskipun semua implementasi berbagi kode umum dan ExecJob()fungsi terlihat lebih manis, ada dua masalah yang mengganggu saya:

• Untuk setiap perubahan CommonTask(), keenam fitur (dan mungkin lebih banyak di masa depan) perlu diuji.
• CommonTask()sudah kompleks. Ini akan menjadi lebih kompleks dari waktu ke waktu.

Apakah saya melakukannya dengan cara yang benar?

Ya, Anda benar-benar di jalan yang benar!

Dalam pengalaman saya, saya perhatikan bahwa ketika segalanya rumit, perubahan terjadi dalam langkah-langkah kecil. Apa yang telah Anda lakukan adalah langkah 1 dalam proses evolusi (atau proses refactoring). Inilah langkah 2 dan langkah 3:

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

Ini adalah 'Pola Desain Templat' dan ini selangkah lebih maju dalam proses refactoring. Jika kelas dasar berubah, subclass (A, B, C) tidak perlu terpengaruh. Anda dapat menambahkan subclass baru dengan relatif mudah. Namun, segera dari gambar di atas Anda dapat melihat bahwa abstraksi rusak. Kebutuhan untuk 'implementasi kosong' adalah indikator yang baik; itu menunjukkan bahwa ada sesuatu yang salah dengan abstraksi Anda. Itu mungkin solusi yang dapat diterima untuk jangka pendek, tetapi tampaknya ada yang lebih baik.

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

Ini adalah 'Pola Desain Strategi' dan tampaknya cocok untuk kasus Anda. Ada berbagai strategi untuk melaksanakan pekerjaan dan setiap kelas (A, B, C) mengimplementasikannya secara berbeda.

Saya yakin ada langkah 4 atau langkah 5 dalam proses ini atau pendekatan refactoring yang jauh lebih baik. Namun, ini akan membuat Anda menghilangkan kode duplikat dan memastikan bahwa perubahan dilokalkan.

Anda sebenarnya melakukan hal yang benar. Saya mengatakan ini karena:

1. Jika Anda perlu mengubah kode untuk fungsi tugas umum, Anda tidak perlu mengubahnya di semua 6 kelas yang akan berisi kode jika Anda tidak menulisnya di kelas umum.
2. Jumlah baris kode akan berkurang.

Anda melihat ini semacam berbagi kode banyak dengan desain event driven (. NET khususnya). Cara yang paling bisa dipelihara adalah menjaga perilaku Anda bersama dalam potongan sekecil mungkin.

Biarkan kode tingkat tinggi menggunakan kembali banyak metode kecil, biarkan kode tingkat tinggi keluar dari basis bersama.

Anda akan memiliki banyak pelat ketel pada implementasi daun / beton Anda. Jangan panik, tidak apa-apa. Semua kode itu langsung, mudah dimengerti. Anda harus mengaturnya sesekali saat barang rusak, tetapi akan mudah diubah.

Anda akan melihat banyak pola dalam kode tingkat tinggi. Terkadang mereka nyata, sebagian besar waktu mereka tidak nyata. "Konfigurasi" dari lima parameter di sana terlihat serupa, tetapi tidak. Mereka adalah tiga strategi yang sama sekali berbeda.

Juga ingin dicatat bahwa Anda dapat melakukan semua ini dengan komposisi dan tidak pernah khawatir tentang warisan. Anda akan memiliki lebih sedikit kopling.

Jika saya adalah Anda, saya mungkin akan menambahkan 1 langkah lagi di awal: studi berbasis UML.

Refactoring kode menggabungkan semua bagian umum bersama tidak selalu merupakan langkah terbaik, terdengar lebih seperti solusi sementara daripada pendekatan yang baik.

Buatlah skema UML, pertahankan hal-hal sederhana namun efektif, ingatlah beberapa konsep dasar tentang proyek Anda seperti "apa yang seharusnya dilakukan perangkat lunak ini?" "Apa cara terbaik untuk menjaga perangkat lunak ini abstrak, modular, dapat dikembangkan, ... dll, dll?" "Bagaimana saya bisa mengimplementasikan enkapsulasi yang terbaik?"

Saya hanya mengatakan ini: jangan pedulikan kode sekarang, Anda hanya perlu peduli dengan logika, ketika Anda memiliki logika yang jelas dalam pikiran semua sisanya dapat menjadi tugas yang sangat mudah, pada akhirnya semua jenis ini masalah yang Anda hadapi hanya disebabkan oleh logika yang buruk.

Langkah pertama, ke mana pun ini pergi, harus memecah metode yang tampaknya besar A::ExecJobmenjadi potongan-potongan kecil.

Karena itu, alih-alih

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
}

Anda mendapatkan

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
}

Mulai dari sini, ada banyak cara yang mungkin untuk dilakukan. Saya mengambilnya: Jadikan A kelas dasar hierachy kelas Anda dan ExecJob virtual dan menjadi mudah untuk membuat B, C, ... tanpa terlalu banyak salin-tempel - cukup ganti ExecJob (sekarang lima-liner) dengan yang dimodifikasi Versi: kapan.

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

Tetapi mengapa ada begitu banyak kelas sama sekali? Mungkin Anda bisa menggantinya semua dengan kelas tunggal yang memiliki konstruktor yang dapat dikatakan tindakan mana yang diperlukan ExecJob.

Saya setuju dengan jawaban lain bahwa pendekatan Anda sejalan, meskipun saya tidak berpikir bahwa pewarisan adalah cara terbaik untuk mengimplementasikan kode umum - saya lebih suka komposisi. Dari C ++ FAQ yang dapat menjelaskannya jauh lebih baik daripada yang pernah saya dapat: http://www.parashift.com/c++-faq/priv-inherit-vs-compos.html

Pertama, Anda harus memastikan bahwa warisan benar-benar alat yang tepat di sini untuk pekerjaan itu - hanya karena Anda memerlukan tempat umum untuk fungsi yang digunakan oleh kelas Anda Auntuk Ftidak berarti bahwa kelas dasar umum adalah hal yang benar di sini - kadang-kadang pembantu terpisah kelas melakukan pekerjaan dengan lebih baik. Mungkin, mungkin juga tidak. Itu tergantung pada memiliki hubungan "is-a" antara A ke F dan kelas dasar Anda, yang tidak mungkin dikatakan dari nama buatan AF. Di sini Anda menemukan posting blog yang berhubungan dengan topik ini.

Mari kita asumsikan Anda memutuskan bahwa kelas dasar umum adalah hal yang benar dalam kasus Anda. Kemudian hal kedua yang akan saya lakukan adalah untuk memastikan Anda fragmen kode S1 untuk S5 masing-masing dilaksanakan dalam metode terpisah S1()untuk S5()kelas dasar Anda. Setelah itu fungsi "ExecJob" akan terlihat seperti ini:

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

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

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

Seperti yang Anda lihat sekarang, karena S1 hingga S5 hanyalah pemanggilan metode, tidak ada blok kode lagi, duplikasi kode telah hampir sepenuhnya dihapus, dan Anda tidak perlu memeriksa parameter lagi, menghindari masalah meningkatnya kompleksitas yang mungkin Anda dapatkan jika tidak.

Akhirnya, tetapi hanya sebagai langkah ketiga (!), Anda mungkin berpikir tentang menggabungkan semua metode ExecJob ke salah satu kelas dasar Anda, di mana pelaksanaan bagian-bagian itu dapat dikontrol oleh parameter, seperti yang Anda sarankan, atau dengan menggunakan pola metode templat. Anda harus memutuskan sendiri apakah itu sepadan dengan usaha Anda, berdasarkan kode nyata.

Tapi IMHO teknik dasar untuk memecah metode besar menjadi metode kecil jauh, jauh lebih penting untuk menghindari duplikasi kode daripada menerapkan pola.