Apakah penggunaan kondisional ini sebagai anti-pola?

Saya sudah sering melihat ini di sistem lawas kami di tempat kerja - fungsi-fungsi yang seperti ini:

bool todo = false;
if(cond1)
{
  ... // lots of code here
  if(cond2)
    todo = true;
  ... // some other code here
}

if(todo)
{
  ...
}

Dengan kata lain, fungsinya memiliki dua bagian. Bagian pertama melakukan semacam pemrosesan (berpotensi mengandung loop, efek samping, dll.), Dan sepanjang jalan itu mungkin mengatur bendera "todo". Bagian kedua hanya dieksekusi jika bendera "todo" telah ditetapkan.

Sepertinya cara yang sangat jelek untuk melakukan sesuatu, dan saya pikir sebagian besar kasus yang saya benar-benar luangkan waktu untuk mengerti, bisa di refactored untuk menghindari penggunaan bendera. Tetapi apakah ini sebenarnya anti-pola, ide yang buruk, atau dapat diterima?

Refactorisasi pertama yang jelas adalah memotongnya menjadi dua metode. Namun, pertanyaan saya lebih lanjut tentang apakah ada kebutuhan (dalam bahasa OO modern) untuk membuat variabel flag lokal, berpotensi mengaturnya di beberapa tempat, dan kemudian menggunakannya untuk memutuskan apakah akan menjalankan blok kode berikutnya.

Saya tidak tahu tentang anti-pola, tapi saya akan mengekstrak tiga metode dari ini.

Yang pertama akan melakukan beberapa pekerjaan dan mengembalikan nilai boolean.

Yang kedua akan melakukan pekerjaan apa pun yang dilakukan oleh "kode lain"

Yang ketiga akan melakukan pekerjaan tambahan jika boolean yang dikembalikan itu benar.

Metode yang diekstraksi mungkin akan bersifat pribadi jika penting bahwa yang kedua hanya (dan selalu) dipanggil jika metode pertama kembali benar.

Dengan menyebutkan metode dengan baik, saya harap ini akan membuat kode lebih jelas.

Sesuatu seperti ini:

public void originalMethod() {
    bool furtherProcessingRequired = lotsOfCode();
    someOtherCode();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

private boolean lotsOfCode() {
    if (cond1) {
        ... // lots of code here
        if(cond2) {
            return true;
        }
    }
    return false;
}

private void someOtherCode() {
    ... // some other code here
}

private void doFurtherProcessing() {
    // Do whatever is needed
}

Jelas ada perdebatan yang bisa didapat tentang apakah pengembalian awal dapat diterima, tetapi itu adalah detail implementasi (seperti standar format kode).

Intinya adalah bahwa maksud kode menjadi lebih jelas, yang bagus ...

Salah satu komentar pada pertanyaan menunjukkan bahwa pola ini mewakili bau , dan saya setuju dengan itu. Anda perlu melihatnya untuk melihat apakah Anda dapat memperjelas maksud tersebut.

Saya pikir keburukan ini disebabkan oleh kenyataan bahwa ada banyak kode dalam satu metode, dan / atau variabel-variabelnya diberi nama buruk (keduanya merupakan bau kode pada hak mereka sendiri - antipemimpin adalah hal-hal yang lebih abstrak dan kompleks IMO).

Jadi jika Anda mengekstrak sebagian besar kode ke metode level yang lebih rendah seperti yang disarankan @Bill, sisanya menjadi bersih (setidaknya untuk saya). Misalnya

bool registrationNeeded = installSoftware(...);
if (registrationNeeded) {
  registerUser(...)
}

Atau Anda bahkan bisa menghilangkan flag lokal sepenuhnya dengan menyembunyikan tanda centang flag ke metode kedua dan menggunakan form like

calculateTaxRefund(isTaxRefundable(...), ...)

Secara keseluruhan, saya tidak melihat memiliki variabel flag lokal sebagai selalu buruk per se. Opsi mana di atas lebih mudah dibaca (= lebih disukai bagi saya) tergantung pada jumlah parameter metode, nama yang dipilih, dan bentuk mana yang lebih konsisten dengan logika bagian dalam kode.

todo adalah nama yang benar-benar buruk untuk variabel, tapi saya pikir mungkin itu yang salah. Sulit untuk sepenuhnya yakin tanpa konteksnya.

Katakanlah bahwa bagian kedua dari fungsi mengurutkan daftar, dibangun oleh bagian pertama. Ini harus jauh lebih mudah dibaca:

bool requiresSorting = false;
if(cond1)
{
    ... // lots of code here
    if(cond2)
        requiresSorting = true;
    ... // some other code here
}

if(requiresSorting)
{
    ...
}

Namun, saran Bill juga benar. Ini masih lebih mudah dibaca:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Pola mesin negara terlihat baik bagi saya. Pola anti di sana adalah "todo" (nama buruk) dan "banyak kode".

Itu sangat tergantung. Jika kode dijaga oleh todo(saya harap Anda tidak menggunakan nama itu nyata karena benar-benar un-mnemonic!) Adalah kode pembersihan secara konseptual, maka Anda memiliki anti-pola dan harus menggunakan sesuatu seperti RAII atau C # dari C ++. usingmembangun untuk menangani hal-hal sebagai gantinya.

Di sisi lain, jika secara konseptual bukan tahap pembersihan tetapi lebih dari sekadar pemrosesan tambahan yang kadang-kadang diperlukan dan di mana keputusan untuk melakukannya perlu diambil lebih awal, apa yang ditulis baik-baik saja. Pertimbangkan apakah masing-masing potongan kode akan lebih baik di-refactored ke dalam fungsinya masing-masing, dan juga apakah Anda telah menangkap makna variabel bendera dalam namanya, tetapi pola kode dasar ini OK. Secara khusus, mencoba memasukkan terlalu banyak ke fungsi lain mungkin membuat apa yang terjadi menjadi kurang jelas, dan itu pasti akan menjadi anti-pola.

Banyak jawaban di sini akan kesulitan melewati pemeriksaan kompleksitas, beberapa terlihat> 10.

Saya pikir ini adalah bagian "anti-pola" dari apa yang Anda lihat. Temukan alat yang mengukur kompleksitas cyclomatic dari kode Anda - ada plugin untuk gerhana. Ini pada dasarnya adalah pengukuran seberapa sulit kode Anda untuk diuji dan melibatkan jumlah dan tingkat cabang kode.

Sebagai tebakan total pada solusi yang mungkin, tata letak jenis kode Anda membuat saya berpikir dalam "Tugas", jika ini terjadi di banyak tempat, mungkin yang benar-benar Anda inginkan adalah arsitektur berorientasi tugas - setiap tugas menjadi miliknya sendiri objek dan di pertengahan tugas Anda memiliki kemampuan untuk melakukan tugas berikutnya dengan membuat instance objek tugas lain dan melemparkannya di antrian. Ini bisa sangat sederhana untuk diatur dan mereka mengurangi kompleksitas jenis kode tertentu secara signifikan - tetapi seperti yang saya katakan ini adalah tikaman total dalam gelap.

Menggunakan contoh pdr di atas, karena ini adalah contoh yang bagus, saya akan melangkah lebih jauh.

Dia punya:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Jadi saya menyadari bahwa yang berikut ini akan berhasil:

if(BuildList(list)) 
    SortList(list)

Tapi tidak jelas.

Jadi untuk pertanyaan awal, mengapa tidak:

BuildList(list)
SortList(list)

Dan biarkan SortList memutuskan apakah perlu disortir?

Anda melihat metode BuildList Anda tampaknya tahu tentang penyortiran, karena ia mengembalikan bool yang menunjukkan hal itu, tetapi itu tidak masuk akal untuk metode yang dirancang untuk membangun daftar.

Ya, ini tampaknya menjadi masalah karena Anda harus terus melacak semua tempat Anda menandai bendera ON / OFF. Lebih baik untuk memasukkan logika hanya di dalam sebagai kondisi bersarang jika daripada mengambil logika.

Juga model domain yang kaya, dalam hal ini hanya satu liner akan melakukan hal-hal besar di dalam objek.

Jika flag hanya diset satu kali maka pindahkan
...
kode ke langsung setelah
... // beberapa kode lain di sini
maka hilangkan dengan flag.

Kalau tidak, pisahkan
... // banyak kode di sini
... // beberapa kode lain di sini
...
kode keluar menjadi fungsi terpisah jika memungkinkan, jadi jelas fungsi ini memiliki satu tanggung jawab yang merupakan logika cabang.

Jika memungkinkan pisahkan kode di dalam
... // banyak kode di sini
menjadi dua fungsi atau lebih, beberapa melakukan beberapa pekerjaan (yang merupakan perintah) dan yang lain mengembalikan nilai todo (yang merupakan permintaan) atau membuatnya sangat eksplisit mereka memodifikasinya (permintaan menggunakan efek samping)

Kode itu sendiri bukan anti-pola yang terjadi di sini ... Saya menduga bahwa percabangan logika percabangan dengan melakukan hal yang sebenarnya (perintah) adalah anti-pola yang Anda cari.

Terkadang saya merasa perlu menerapkan pola ini. Terkadang Anda ingin melakukan beberapa pemeriksaan sebelum melanjutkan operasi. Untuk alasan efisiensi, perhitungan yang melibatkan pemeriksaan tertentu tidak dilakukan kecuali jika tampaknya benar-benar perlu untuk memeriksa. Jadi Anda biasanya melihat kode seperti:

// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(valuesExist) {
    try {
      // Attempt insertion
      trx.commit();
    } catch (DatabaseException dbe) {
      trx.rollback();
      throw dbe;
    }
  } else {
    closeConnection(db);
    throwException();
  }
} else {
  closeConnection(db);
  throwException();
}

Ini dapat disederhanakan dengan memisahkan validasi dari proses aktual menjalankan operasi, sehingga Anda akan melihat lebih seperti:

boolean proceed = true;
// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(!valuesExist) {
    proceed = false;
  }
} else {
  proceed = false;
}

// The moment of truth
if(proceed) {
  try {
    // Attempt insertion
    trx.commit();
  } catch (DatabaseException dbe) {
    trx.rollback();
    throw dbe;
  }
} else {
  if(db.isOpen()) {
    closeConnection(db);
  }
  throwException();
}

Jelas itu bervariasi sesuai dengan apa yang Anda coba capai, meskipun ditulis seperti ini, baik kode "sukses" dan "kegagalan" Anda ditulis satu kali, yang menyederhanakan logika dan mempertahankan tingkat kinerja yang sama. Dari sana, akan menjadi ide bagus untuk menyesuaikan seluruh level validasi di dalam metode internal yang mengembalikan indikasi boolean keberhasilan atau kegagalan yang selanjutnya menyederhanakan banyak hal, meskipun beberapa programmer menyukai metode yang sangat panjang karena alasan yang aneh.

Ini bukan sebuah pola . Interpretasi paling umum adalah bahwa Anda menetapkan variabel boolean dan bercabang pada nilainya nanti. Itu adalah pemrograman prosedural normal, tidak lebih.

Sekarang, contoh spesifik Anda dapat ditulis ulang sebagai:

if(cond1)
{
    ... // lots of code here
    ... // some other code here
    if (cond2)
    {
        ...
    }
}

Itu mungkin lebih mudah dibaca. Atau mungkin tidak. Itu tergantung pada sisa kode yang Anda hapus. Fokus untuk membuat kode itu lebih ringkas.

Bendera lokal yang digunakan untuk aliran kontrol harus dikenali sebagai bentuk goto penyamaran. Jika sebuah bendera hanya digunakan dalam suatu fungsi, itu bisa dihilangkan dengan menuliskan dua salinan dari fungsi tersebut, memberi label satu sebagai "bendera itu benar" dan yang lain sebagai "bendera itu salah", dan setelah mengganti setiap operasi yang menetapkan bendera tersebut ketika itu jelas, atau menghapusnya ketika sudah diatur, dengan lompatan antara dua versi fungsi.

Dalam banyak kasus, kode yang menggunakan bendera akan lebih bersih daripada alternatif lain yang mungkin digunakan goto, tetapi itu tidak selalu benar. Dalam beberapa kasus, menggunakangoto untuk melewati sepotong kode mungkin lebih bersih daripada menggunakan bendera untuk melakukannya [meskipun beberapa orang mungkin memasukkan kartun raptor tertentu di sini].

Saya pikir prinsip panduan utama adalah aliran logika program harus menyerupai deskripsi logika bisnis sejauh mungkin. Jika persyaratan logika bisnis dijelaskan dalam bentuk kondisi yang dipisah dan digabungkan dengan cara yang aneh, membuat logika program juga mungkin lebih bersih daripada mencoba menggunakan flag untuk menyembunyikan logika tersebut. Di sisi lain, jika cara paling alami untuk menggambarkan aturan bisnis adalah dengan mengatakan bahwa suatu tindakan harus dilakukan jika tindakan tertentu lainnya telah dilakukan, cara paling alami untuk mengekspresikan yang mungkin menggunakan bendera yang ditetapkan ketika melakukan tindakan terakhir dan sebaliknya jelas.