Apakah ekspresi boolean besar lebih mudah dibaca daripada ekspresi yang sama dipecah menjadi metode predikat? [Tutup]

Apa yang lebih mudah dipahami, pernyataan boolean besar (cukup kompleks), atau pernyataan yang sama dipecah menjadi metode predikat (banyak kode tambahan untuk dibaca)?

Opsi 1, ekspresi boolean besar:

    private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
    {

        return propVal.PropertyId == context.Definition.Id
            && !repo.ParentId.HasValue || repo.ParentId == propVal.ParentId
            && ((propVal.SecondaryFilter.HasValue && context.SecondaryFilter.HasValue && propVal.SecondaryFilter.Value == context.SecondaryFilter) || (!context.SecondaryFilter.HasValue && !propVal.SecondaryFilter.HasValue));
    }

Opsi 2, Kondisi dipecah menjadi metode predikat:

    private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
    {
        return MatchesDefinitionId(context, propVal)
            && MatchesParentId(propVal)
            && (MatchedSecondaryFilter(context, propVal) || HasNoSecondaryFilter(context, propVal));
    }

    private static bool HasNoSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);
    }

    private static bool MatchedSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    }

    private bool MatchesParentId(TValToMatch propVal)
    {
        return (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    }

    private static bool MatchesDefinitionId(CurrentSearchContext context, TValToMatch propVal)
    {
        return propVal.PropertyId == context.Definition.Id;
    }

Saya lebih suka pendekatan kedua, karena saya melihat nama metode sebagai komentar, tetapi saya mengerti bahwa itu bermasalah karena Anda harus membaca semua metode untuk memahami apa yang kode lakukan, sehingga abstrak maksud kode.

Apa yang lebih mudah dimengerti

Pendekatan yang terakhir. Ini tidak hanya lebih mudah dimengerti tetapi juga lebih mudah untuk menulis, menguji, refactor dan memperluas juga. Setiap kondisi yang diperlukan dapat dipisahkan secara aman dan ditangani dengan caranya sendiri.

itu bermasalah karena Anda harus membaca semua metode untuk memahami kode

Tidak masalah jika metode diberi nama dengan benar. Bahkan akan lebih mudah dipahami karena nama metode akan menggambarkan maksud dari kondisi.
Untuk seorang penonton if MatchesDefinitionId()lebih jelas daripadaif (propVal.PropertyId == context.Definition.Id)

[Secara pribadi, pendekatan pertama membuat mataku sakit.]

Jika ini adalah satu-satunya tempat fungsi predikat ini akan digunakan, Anda juga dapat menggunakan boolvariabel lokal sebagai gantinya:

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    bool matchesDefinitionId = (propVal.PropertyId == context.Definition.Id);
    bool matchesParentId = (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    bool matchesSecondaryFilter = (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    bool hasNoSecondaryFilter = (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);

    return matchesDefinitionId
        && matchesParentId
        && matchesSecondaryFilter || hasNoSecondaryFilter;
}

Ini juga dapat dipecah lebih lanjut dan disusun ulang untuk membuatnya lebih mudah dibaca, misalnya dengan

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

dan kemudian mengganti semua instance dari propVal.SecondaryFilter.HasValue. Satu hal yang segera menonjol adalah yang hasNoSecondaryFiltermenggunakan logika AND pada HasValueproperti yang dinegasikan , sementara matchesSecondaryFiltermenggunakan AND yang logis pada yang tidak dinegasikan HasValue- jadi bukan sebaliknya.

Secara umum, yang terakhir lebih disukai.

Itu membuat situs panggilan lebih dapat digunakan kembali. Ini mendukung KERING (artinya Anda memiliki lebih sedikit tempat untuk berubah ketika kriteria berubah, dan dapat melakukannya dengan lebih andal). Dan sangat sering sub-kriteria itu adalah hal-hal yang akan digunakan kembali secara independen di tempat lain, memungkinkan Anda untuk melakukan itu.

Oh, dan ini membuat hal ini jauh lebih mudah untuk unit test, memberi Anda kepercayaan diri bahwa Anda telah melakukannya dengan benar.

Jika ada di antara dua pilihan ini, maka yang terakhir lebih baik. Namun, ini bukan satu-satunya pilihan! Bagaimana dengan memecah fungsi tunggal menjadi beberapa ifs? Tes cara-cara untuk keluar dari fungsi untuk menghindari tes tambahan, kira-kira meniru "hubungan pendek" dalam tes jalur tunggal.

Ini lebih mudah dibaca (Anda mungkin perlu memeriksa logika untuk contoh Anda, tetapi konsepnya benar):

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    if( propVal.PropertyId != context.Definition.Id ) return false;

    if( repo.ParentId.HasValue || repo.ParentId != propVal.ParentId ) return false;

    if( propVal.SecondaryFilter.HasValue && 
        context.SecondaryFilter.HasValue && 
        propVal.SecondaryFilter.Value == context.SecondaryFilter ) return true;

    if( !context.SecondaryFilter.HasValue && 
        !propVal.SecondaryFilter.HasValue) return true;

    return false;   
}

Saya suka opsi 2 lebih baik, tetapi akan menyarankan satu perubahan struktural. Gabungkan kedua cek pada baris terakhir dari conditional menjadi satu panggilan.

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    return MatchesDefinitionId(context, propVal)
        && MatchesParentId(propVal)
        && MatchesSecondaryFilterIfPresent(context, propVal);
}

private static bool MatchesSecondaryFilterIfPresent(CurrentSearchContext context, 
                                                    TValToMatch propVal)
{
    return MatchedSecondaryFilter(context, propVal) 
               || HasNoSecondaryFilter(context, propVal);
}

Alasan saya menyarankan ini adalah bahwa dua pemeriksaan adalah unit fungsional tunggal, dan tanda kurung bersarang dalam kondisi adalah rentan kesalahan: Baik dari sudut pandang awalnya menulis kode dan dari sudut pandang orang yang membacanya. Ini terutama terjadi jika sub-elemen dari ekspresi tidak mengikuti pola yang sama.

Saya tidak yakin apakah MatchesSecondaryFilterIfPresent()itu nama terbaik untuk kombinasi; tetapi tidak ada yang lebih baik segera muncul dalam pikiran.

Meskipun dalam C #, kode tidak terlalu berorientasi objek. Itu menggunakan metode statis dan apa yang tampak seperti bidang statis (misalnya repo). Secara umum diyakini bahwa statika membuat kode Anda sulit untuk diperbaiki dan sulit untuk diuji, sementara menghambat penggunaan kembali, dan, untuk pertanyaan Anda: penggunaan statis seperti ini kurang dapat dibaca & dipelihara daripada konstruksi berorientasi objek.

Anda harus mengonversi kode ini ke bentuk yang lebih berorientasi objek. Ketika Anda melakukannya, Anda akan menemukan bahwa ada tempat yang masuk akal untuk meletakkan kode yang melakukan perbandingan objek, bidang, dll. Kemungkinan Anda kemudian dapat meminta objek untuk membandingkan diri mereka sendiri, yang akan mengurangi besar Anda jika pernyataan ke permintaan sederhana untuk membandingkan (misalnya if ( a.compareTo (b) ) { }, yang dapat mencakup semua perbandingan bidang.)

C # memiliki banyak antarmuka dan utilitas sistem untuk melakukan perbandingan pada objek dan bidangnya. Di luar jelas .Equalsmetode, untuk pemula, melihat ke dalam IEqualityComparer, IEquatabledan utilitas seperti System.Collections.Generic.EqualityComparer.Default.

Yang terakhir ini jelas disukai, saya telah melihat kasus dengan cara pertama dan hampir selalu mustahil untuk dibaca. Saya telah melakukan kesalahan dengan melakukannya dengan cara pertama dan diminta untuk mengubahnya menjadi metode predikat.

Saya akan mengatakan bahwa keduanya hampir sama, JIKA Anda menambahkan beberapa ruang kosong untuk dibaca dan beberapa komentar untuk membantu pembaca untuk bagian yang lebih tidak jelas.

Ingat: komentar yang baik memberi tahu pembaca apa yang Anda pikirkan ketika Anda menulis kode.

Dengan perubahan seperti yang saya sarankan, saya mungkin akan pergi dengan pendekatan sebelumnya, karena kurang berantakan dan menyebar. Panggilan subrutin seperti catatan kaki: mereka memberikan informasi yang bermanfaat tetapi mengganggu aliran bacaan. Jika predikatnya lebih kompleks, maka saya akan memecahnya menjadi metode terpisah sehingga konsep yang mereka wujudkan dapat dibangun dalam potongan yang dapat dimengerti.

Nah, jika ada bagian-bagian yang mungkin ingin Anda gunakan kembali, memisahkannya menjadi fungsi-fungsi yang disebut dengan benar jelas merupakan ide terbaik.
Bahkan jika Anda mungkin tidak pernah menggunakannya kembali, melakukan hal itu memungkinkan Anda untuk menyusun kondisi Anda dengan lebih baik dan memberi mereka label yang menjelaskan apa artinya .

Sekarang, mari kita lihat opsi pertama Anda, dan mengakui bahwa tidak ada lekukan Anda dan pemecah baris semua yang berguna, juga kondisional tidak terstruktur dengan baik:

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal) {
    return propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue
        || repo.ParentId == propVal.ParentId
        && propVal.SecondaryFilter.HasValue == context.SecondaryFilter.HasValue
        && (!propVal.SecondaryFilter.HasValue || propVal.SecondaryFilter.Value == context.SecondaryFilter.Value);
}

Yang pertama benar-benar mengerikan. Anda telah menggunakan || untuk dua hal pada baris yang sama; itu adalah bug dalam kode Anda atau niat untuk mengaburkan kode Anda.

    return (   (   propVal.PropertyId == context.Definition.Id
                && !repo.ParentId.HasValue)
            || (   repo.ParentId == propVal.ParentId
                && (   (   propVal.SecondaryFilter.HasValue
                        && context.SecondaryFilter.HasValue 
                        && propVal.SecondaryFilter.Value == context.SecondaryFilter)
                    || (   !context.SecondaryFilter.HasValue
                        && !propVal.SecondaryFilter.HasValue))));

Setidaknya setengah jalan diformat dengan sopan (jika formatnya rumit, itu karena jika-kondisi rumit), dan Anda setidaknya memiliki kesempatan untuk mencari tahu apakah ada sesuatu di sana omong kosong. Dibandingkan dengan sampah Anda yang diformat jika, ada yang lebih baik. Tetapi Anda tampaknya hanya mampu melakukan ekstrem: Entah kekacauan pernyataan if, atau empat metode yang sama sekali tidak berguna.

Perhatikan bahwa (cond1 && cond2) || (! cond1 && cond3) dapat ditulis sebagai

cond1 ? cond2 : cond3

yang akan mengurangi kekacauan. Saya akan menulis

if (propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue) {
    return true;
} else if (repo.ParentId != propVal.ParentId) {
    return false;
} else if (propVal.SecondaryFilter.HasValue) {
    return (   context.SecondaryFilter.HasValue
            && propVal.SecondaryFilter.Value == context.SecondaryFilter); 
} else {
    return !context.SecondaryFilter.HasValue;
}

Saya tidak suka salah satu dari solusi itu, keduanya sulit untuk dipikirkan, dan sulit dibaca. Abstraksi ke metode yang lebih kecil hanya demi metode yang lebih kecil tidak selalu menyelesaikan masalah.

Idealnya, saya pikir Anda metaprogrmatically membandingkan properti, sehingga Anda tidak memiliki mendefinisikan metode baru atau jika cabang setiap kali Anda ingin membandingkan satu set properti baru.

Saya tidak yakin tentang c #, tetapi dalam javascript sesuatu seperti ini akan JAUH lebih baik dan setidaknya bisa menggantikan MatchesDefinitionId dan MatchesParentId

function compareContextProp(obj, property, value){
  if(obj[property])
    return obj[property] == value
  return false
}