Apakah ini bau kode untuk mengatur bendera dalam satu lingkaran untuk menggunakannya nanti?

Saya memiliki sepotong kode di mana saya mengulangi peta sampai kondisi tertentu benar dan kemudian menggunakan kondisi itu untuk melakukan beberapa hal lagi.

Contoh:

Map<BigInteger, List<String>> map = handler.getMap();

if(map != null && !map.isEmpty())
{
    for (Map.Entry<BigInteger, List<String>> entry : map.entrySet())
    {
        fillUpList();

        if(list.size() > limit)
        {
            limitFlag = true;
            break;
        }
    }
}
else
{
    logger.info("\n>>>>> \n\t 6.1 NO entries to iterate over (for given FC and target) \n");
}

if(!limitFlag) // Continue only if limitFlag is not set
{
    // Do something
}

Saya merasa menetapkan bendera dan kemudian menggunakannya untuk melakukan lebih banyak hal adalah bau kode.

Apakah saya benar? Bagaimana saya bisa menghapus ini?

Tidak ada yang salah dengan menggunakan nilai Boolean untuk tujuan yang dimaksudkan: untuk merekam perbedaan biner.

Jika saya disuruh refactor kode ini, saya mungkin akan memasukkan loop ke dalam metode sendiri sehingga + tugas breakmenjadi + return; maka Anda bahkan tidak perlu variabel, Anda bisa mengatakannya

if(fill_list_from_map()) {
  ...

Itu tidak selalu buruk, dan kadang-kadang itu adalah solusi terbaik. Tetapi pengaturan bendera seperti ini di blok bersarang dapat membuat kode sulit untuk diikuti.

Masalahnya adalah Anda memiliki blok untuk membatasi cakupan, tetapi kemudian Anda memiliki bendera yang berkomunikasi lintas cakupan, melanggar isolasi logis dari blok. Sebagai contoh, itu limitFlagakan menjadi false jika mapis null, jadi "do something" -code akan dieksekusi if mapis null. Ini mungkin yang Anda maksudkan, tetapi itu bisa berupa bug yang mudah terlewatkan, karena kondisi untuk bendera ini ditentukan di tempat lain, di dalam lingkup bersarang. Jika Anda dapat menyimpan informasi dan logika di dalam ruang lingkup seketat mungkin, Anda harus berusaha melakukannya.

Saya akan menyarankan agar tidak berpikir tentang 'kode bau'. Itu hanya cara paling malas untuk merasionalisasi bias Anda sendiri. Seiring waktu Anda akan mengembangkan banyak bias, dan banyak dari mereka akan masuk akal, tetapi banyak dari mereka akan menjadi bodoh.

Sebagai gantinya, Anda harus memiliki alasan praktis (yaitu, bukan dogmatis) untuk lebih memilih satu hal daripada yang lain, dan menghindari berpikir bahwa Anda harus memiliki jawaban yang sama untuk semua pertanyaan serupa.

"Kode berbau" adalah untuk saat Anda tidak berpikir. Jika Anda benar-benar akan memikirkan kode itu, maka lakukan dengan benar!

Dalam hal ini, keputusan dapat benar-benar berjalan baik tergantung pada kode di sekitarnya. Itu benar-benar tergantung pada apa yang Anda pikirkan adalah cara paling jelas untuk berpikir tentang apa yang dilakukan kode. (kode "bersih" adalah kode yang dengan jelas mengomunikasikan apa yang dilakukannya ke pengembang lain dan membuatnya mudah bagi mereka untuk memverifikasi bahwa itu benar)

Sering kali, orang akan menulis metode yang terstruktur menjadi beberapa fase, di mana kode pertama-tama akan menentukan apa yang perlu diketahui tentang data dan kemudian bertindak berdasarkan hal itu. Jika bagian "tentukan" dan bagian "lakari" keduanya sedikit rumit, maka masuk akal untuk melakukan ini, dan sering kali "apa yang perlu diketahui" dapat dilakukan di antara fase dalam bendera Boolean. Saya benar-benar lebih suka Anda memberi bendera nama yang lebih baik. Sesuatu seperti "largeEntryExists" akan membuat kode lebih bersih.

Di lain pihak, jika kode "// Do Something" sangat sederhana, maka lebih masuk akal untuk meletakkannya di dalam ifblok alih-alih mengatur bendera. Itu menempatkan efek lebih dekat ke penyebabnya, dan pembaca tidak harus memindai sisa kode untuk memastikan bahwa flag mempertahankan nilai yang Anda tetapkan.

Ya, ini adalah bau kode (isyarat turun dari semua orang yang melakukannya).

Kuncinya bagi saya adalah penggunaan breakpernyataan itu. Jika Anda tidak menggunakannya maka Anda akan mengulangi item lebih dari yang dibutuhkan, tetapi menggunakannya memberikan dua kemungkinan titik keluar dari loop.

Bukan masalah besar dengan contoh Anda, tetapi Anda dapat membayangkan bahwa sebagai conditional atau conditional di dalam loop menjadi lebih kompleks atau urutan daftar awal menjadi penting maka lebih mudah bagi bug untuk merangkak ke dalam kode.

Ketika kode sesederhana contoh Anda, dapat dikurangi menjadi satu whilelingkaran atau peta yang setara, membangun filter.

Ketika kode ini cukup kompleks untuk membutuhkan flag dan break maka akan rentan terhadap bug.

Seperti halnya semua kode berbau: Jika Anda melihat bendera, coba ganti dengan a while. Jika tidak bisa, tambahkan tes unit tambahan.

Cukup gunakan nama selain limitFlag yang memberitahu apa yang sebenarnya Anda periksa. Dan mengapa Anda mencatat apa pun saat peta tidak ada atau kosong? limtFlag akan salah, semua yang Anda pedulikan. Loopnya baik-baik saja jika peta kosong, jadi tidak perlu memeriksa itu.

Menetapkan nilai boolean untuk menyampaikan informasi yang sudah Anda miliki merupakan praktik buruk menurut saya. Jika tidak ada alternatif yang mudah maka itu mungkin menunjukkan masalah yang lebih besar seperti enkapsulasi yang buruk.

Anda harus memindahkan logika loop ke metode fillUpList agar dapat rusak jika batas tercapai. Kemudian periksa ukuran daftar secara langsung sesudahnya.

Jika itu merusak kode Anda, mengapa?

Pertama dari kasus umum: Menggunakan bendera untuk memeriksa apakah beberapa elemen koleksi memenuhi kondisi tertentu tidak biasa. Tetapi pola yang paling sering saya lihat untuk menyelesaikan ini adalah dengan memindahkan cek dalam metode tambahan dan langsung kembali darinya (seperti yang dijelaskan oleh Kilian Foth dalam jawabannya ):

private <T> boolean checkCollection(Collection<T> collection)
{
    for (T element : collection)
        if (checkElement(element))
            return true;
    return false;
}

Karena Java 8 ada cara yang lebih ringkas menggunakan Stream.anyMatch(…):

collection.stream().anyMatch(this::checkElement);

Dalam kasus Anda, ini mungkin akan terlihat seperti ini (dengan asumsi list == entry.getValue()dalam pertanyaan Anda):

map.values().stream().anyMatch(list -> list.size() > limit);

Masalah dalam contoh spesifik Anda adalah panggilan tambahan untuk fillUpList(). Jawabannya sangat tergantung pada apa yang seharusnya dilakukan metode ini.

Catatan: Saat berdiri, panggilan untuk fillUpList()tidak masuk akal, karena tidak bergantung pada elemen yang saat ini Anda gunakan. Saya kira ini adalah konsekuensi dari melucuti kode Anda yang sebenarnya agar sesuai dengan format pertanyaan. Tapi justru itu mengarah pada contoh buatan yang sulit diartikan dan karenanya sulit dipikirkan. Oleh karena itu sangat penting untuk memberikan Minimal, Lengkap , dan diverifikasi contoh .

Jadi saya berasumsi bahwa kode aktual melewati arus entryke metode.

Tetapi ada beberapa pertanyaan lagi:

• Apakah daftar di peta kosong sebelum mencapai kode ini? Jika demikian, mengapa sudah ada peta dan bukan hanya daftar atau set BigIntegerkunci? Jika tidak kosong, mengapa Anda perlu mengisi daftar? Ketika sudah ada elemen dalam daftar, bukankah itu pembaruan atau perhitungan lain dalam kasus ini?
• Apa yang menyebabkan daftar menjadi lebih besar dari batas? Apakah ini kondisi kesalahan atau diharapkan sering terjadi? Apakah itu disebabkan oleh input yang tidak valid?
• Apakah Anda memerlukan daftar yang dihitung hingga Anda mencapai daftar yang lebih besar dari batas?
• Apa yang dilakukan bagian " Lakukan sesuatu "?
• Apakah Anda memulai kembali pengisian setelah bagian ini?

Ini hanya beberapa pertanyaan yang muncul di benak saya ketika saya mencoba memahami fragmen kode. Jadi, menurut saya, itu adalah bau kode asli : Kode Anda tidak secara jelas mengomunikasikan maksudnya.

Ini bisa berarti ini ("semua atau tidak sama sekali" dan mencapai batas mengindikasikan kesalahan):

/**
 * Computes the list of all foo strings for each passed number.
 * 
 * @param numbers the numbers to process. Must not be {@code null}.
 * @return all foo strings for each passed number. Never {@code null}.
 * @throws InvalidArgumentException if any number produces a list that is too long.
 */
public Map<BigInteger, List<String>> computeFoos(Set<BigInteger> numbers)
        throws InvalidArgumentException
{
    if (numbers.isEmpty())
    {
        // Do you actually need to log this here?
        // The caller might know better what to do in this case...
        logger.info("Nothing to compute");
    }
    return numbers.stream().collect(Collectors.toMap(
            number -> number,
            number -> computeListForNumber(number)));
}

private List<String> computeListForNumber(BigInteger number)
        throws InvalidArgumentException
{
    // compute the list and throw an exception if the limit is exceeded.
}

Atau itu bisa berarti ini ("perbarui hingga masalah pertama"):

/**
 * Refreshes all foo lists after they have become invalid because of bar.
 * 
 * @param map the numbers with all their current values.
 *            The values in this map will be modified.
 *            Must not be {@code null}.
 * @throws InvalidArgumentException if any new foo list would become too long.
 *             Some other lists may have already been updated.
 */
public void updateFoos(Map<BigInteger, List<String>> map)
        throws InvalidArgumentException
{
    map.replaceAll(this::computeUpdatedList);
}

private List<String> computeUpdatedList(
        BigInteger number, List<String> currentValues)
        throws InvalidArgumentException
{
    // compute the new list and throw an exception if the limit is exceeded.
}

Atau ini ("perbarui semua daftar tetapi simpan daftar asli jika terlalu besar"):

/**
 * Refreshes all foo lists after they have become invalid because of bar.
 * Lists that would become too large will not be updated.
 * 
 * @param map the numbers with all their current values.
 *            The values in this map will be modified.
 *            Must not be {@code null}.
 * @return {@code true} if all updates have been successful,
 *         {@code false} if one or more elements have been skipped
 *         because the foo list size limit has been reached.
 */
public boolean updateFoos(Map<BigInteger, List<String>> map)
{
    boolean allUpdatesSuccessful = true;
    for (Entry<BigInteger, List<String>> entry : map.entrySet())
    {
        List<String> newList = computeListForNumber(entry.getKey());
        if (newList.size() > limit)
            allUpdatesSuccessful = false;
        else
            entry.setValue(newList);
    }
    return allUpdatesSuccessful;
}

private List<String> computeListForNumber(BigInteger number)
{
    // compute the new list
}

Atau bahkan yang berikut (menggunakan computeFoos(…)dari contoh pertama tetapi tanpa pengecualian):

/**
 * Processes the passed numbers. An optimized algorithm will be used if any number
 * produces a foo list of a size that justifies the additional overhead.
 * 
 * @param numbers the numbers to process. Must not be {@code null}.
 */
public void process(Collection<BigInteger> numbers)
{
    Map<BigInteger, List<String>> map = computeFoos(numbers);
    if (isLimitReached(map))
        processLarge(map);
    else
        processSmall(map);
}

private boolean isLimitReached(Map<BigInteger, List<String>> map)
{
    return map.values().stream().anyMatch(list -> list.size() > limit);
}

Atau itu bisa berarti sesuatu yang sama sekali berbeda ... ;-)