Hindari metode yang terlalu rumit - Kompleksitas Siklomatik

Tidak yakin bagaimana cara metode ini untuk mengurangi Kompleksitas Siklomatik. Sonar melaporkan 13 sedangkan 10 diharapkan. Saya yakin tidak ada salahnya meninggalkan metode ini sebagaimana adanya, hanya menantang saya bagaimana cara mematuhi aturan Sonar. Pikiran apa pun akan sangat dihargai.

 public static long parseTimeValue(String sValue) {

    if (sValue == null) {
        return 0;
    }

    try {
        long millis;
        if (sValue.endsWith("S")) {
            millis = new ExtractSecond(sValue).invoke();
        } else if (sValue.endsWith("ms")) {
            millis = new ExtractMillisecond(sValue).invoke();
        } else if (sValue.endsWith("s")) {
            millis = new ExtractInSecond(sValue).invoke();
        } else if (sValue.endsWith("m")) {
            millis = new ExtractInMinute(sValue).invoke();
        } else if (sValue.endsWith("H") || sValue.endsWith("h")) {
            millis = new ExtractHour(sValue).invoke();
        } else if (sValue.endsWith("d")) {
            millis = new ExtractDay(sValue).invoke();
        } else if (sValue.endsWith("w")) {
            millis = new ExtractWeek(sValue).invoke();
        } else {
            millis = Long.parseLong(sValue);
        }

        return millis;

    } catch (NumberFormatException e) {
        LOGGER.warn("Number format exception", e);
    }

    return 0;
}

Semua metode ExtractXXX didefinisikan sebagai statickelas dalam. Misalnya, seperti di bawah ini -

    private static class ExtractHour {
      private String sValue;

      public ExtractHour(String sValue) {
         this.sValue = sValue;
      }

      public long invoke() {
         long millis;
         millis = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * 60 * 60 * 1000);
         return millis;
     }
 }

PEMBARUAN 1

Saya akan puas dengan campuran saran di sini untuk memuaskan pria Sonar. Pasti ruang untuk perbaikan dan penyederhanaan.

Jambu Functionhanya upacara yang tidak diinginkan di sini. Ingin memperbarui pertanyaan tentang status saat ini. Tidak ada yang final di sini. Tolong tuangkan pikiran Anda ..

public class DurationParse {

private static final Logger LOGGER = LoggerFactory.getLogger(DurationParse.class);
private static final Map<String, Function<String, Long>> MULTIPLIERS;
private static final Pattern STRING_REGEX = Pattern.compile("^(\\d+)\\s*(\\w+)");

static {

    MULTIPLIERS = new HashMap<>(7);

    MULTIPLIERS.put("S", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractSecond(input).invoke();
        }
    });

    MULTIPLIERS.put("s", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractInSecond(input).invoke();
        }
    });

    MULTIPLIERS.put("ms", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractMillisecond(input).invoke();
        }
    });

    MULTIPLIERS.put("m", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractInMinute(input).invoke();
        }
    });

    MULTIPLIERS.put("H", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractHour(input).invoke();
        }
    });

    MULTIPLIERS.put("d", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractDay(input).invoke();
        }
    });

    MULTIPLIERS.put("w", new Function<String, Long>() {
        @Nullable
        @Override
        public Long apply(@Nullable String input) {
            return new ExtractWeek(input).invoke();
        }
    });

}

public static long parseTimeValue(String sValue) {

    if (isNullOrEmpty(sValue)) {
        return 0;
    }

    Matcher matcher = STRING_REGEX.matcher(sValue.trim());

    if (!matcher.matches()) {
        LOGGER.warn(String.format("%s is invalid duration, assuming 0ms", sValue));
        return 0;
    }

    if (MULTIPLIERS.get(matcher.group(2)) == null) {
        LOGGER.warn(String.format("%s is invalid configuration, assuming 0ms", sValue));
        return 0;
    }

    return MULTIPLIERS.get(matcher.group(2)).apply(matcher.group(1));
}

private static class ExtractSecond {
    private String sValue;

    public ExtractSecond(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = Long.parseLong(sValue);
        return millis;
    }
}

private static class ExtractMillisecond {
    private String sValue;

    public ExtractMillisecond(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue));
        return millis;
    }
}

private static class ExtractInSecond {
    private String sValue;

    public ExtractInSecond(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 1000);
        return millis;
    }
}

private static class ExtractInMinute {
    private String sValue;

    public ExtractInMinute(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 60 * 1000);
        return millis;
    }
}

private static class ExtractHour {
    private String sValue;

    public ExtractHour(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 60 * 60 * 1000);
        return millis;
    }
}

private static class ExtractDay {
    private String sValue;

    public ExtractDay(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 24 * 60 * 60 * 1000);
        return millis;
    }
}

private static class ExtractWeek {
    private String sValue;

    public ExtractWeek(String sValue) {
        this.sValue = sValue;
    }

    public long invoke() {
        long millis;
        millis = (long) (Double.parseDouble(sValue) * 7 * 24 * 60 * 60 * 1000);
        return millis;
    }
}

}

PEMBARUAN 2

Meskipun saya menambahkan pembaruan saya, itu hanya sepadan dengan waktu. Saya akan pindah karena Sonar sekarang tidak mengeluh. Jangan terlalu khawatir dan saya menerima jawaban mattnz karena itu adalah cara untuk pergi dan tidak ingin memberikan contoh yang buruk bagi mereka yang menabrak pertanyaan ini. Intinya - Jangan over engineer demi Sonar (atau Manajer Proyek Setengah Baked) mengeluh tentang CC. Lakukan saja apa yang bernilai satu sen untuk proyek ini. Terimakasih untuk semua.

Jawaban Rekayasa Perangkat Lunak:

Ini hanya salah satu dari banyak kasus di mana hanya menghitung kacang yang mudah dihitung akan membuat Anda melakukan hal yang salah. Ini bukan fungsi yang kompleks, jangan mengubahnya. Kompleksitas Siklomatik hanyalah panduan untuk kompleksitas, dan Anda menggunakannya dengan buruk jika Anda mengubah fungsi ini berdasarkan itu. Sederhana, mudah dibaca, dapat dipelihara (untuk saat ini), jika semakin besar di masa mendatang, CC akan meroket secara eksponensial dan akan mendapatkan perhatian yang dibutuhkannya saat dibutuhkan, bukan sebelumnya.

Minion bekerja untuk Perusahaan Multinasional Besar. Jawaban:

Organisasi penuh dengan tim penghitung kacang yang dibayar lebih tinggi dan tidak produktif. Menjaga konter kacang tetap lebih mudah, dan tentu saja lebih bijaksana, daripada melakukan hal yang benar. Anda perlu mengubah rutinitas untuk menurunkan CC menjadi 10, tetapi jujurlah mengapa Anda melakukannya - untuk menjaga agar penghitung kacang tidak ada di belakang Anda. Seperti yang disarankan dalam komentar - "parser monadik" mungkin membantu

Terima kasih kepada @JimmyHoffa, @MichaelT, dan @ GlenH7 untuk bantuan mereka!

Python

Hal pertama yang pertama, Anda harus benar-benar hanya menerima awalan yang dikenal, yaitu 'H' atau 'h'. Jika Anda harus menerima keduanya, Anda harus melakukan beberapa operasi agar konsisten untuk menghemat ruang di peta Anda.

Dengan python Anda bisa membuat kamus.

EXTRACTION_MAP = {
    'S': ExtractSecond,
    'ms': ExtractMillisecond,
    'm': ExtractMinute,
    'H': ExtractHour,
    'd': ExtractDay,
    'w': ExtractWeek
}

Maka kami ingin metode untuk menggunakan ini:

def parseTimeValue(sValue)
    ending = ''.join([i for i in sValue if not i.isdigit()])
    return EXTRACTION_MAP[ending](sValue).invoke()

Seharusnya memiliki kompleksitas siklomatik yang lebih baik.

Jawa

Kami hanya membutuhkan 1 (satu) dari setiap pengali. Mari kita meletakkannya di peta seperti yang disarankan oleh beberapa jawaban lain.

Map<String, Float> multipliers = new HashMap<String, Float>();
    map.put("S", 60 * 60);
    map.put("ms", 60 * 60 * 1000);
    map.put("m", 60);
    map.put("H", 1);
    map.put("d", 1.0 / 24);
    map.put("w", 1.0 / (24 * 7));

Lalu kita bisa menggunakan peta untuk mengambil konverter yang tepat

Pattern foo = Pattern.compile(".*(\\d+)\\s*(\\w+)");
Matcher bar = foo.matcher(sValue);
if(bar.matches()) {
    return (long) (Double.parseDouble(bar.group(1)) * multipliers.get(bar.group(2);
}

Karena Anda return millispada akhirnya ifelseifelse yang mengerikan itu, hal pertama yang terlintas dalam pikiran adalah mengembalikan nilai segera dari dalam if-block. Pendekatan ini mengikuti yang tercantum dalam katalog pola refactoring sebagai Ganti Bersarang Bersyarat dengan Klausa Penjaga .

Suatu metode memiliki perilaku kondisional yang tidak memperjelas apa jalur eksekusi normal

Gunakan Klausul Penjaga untuk semua kasus khusus

Ini akan membantu Anda menyingkirkan yang lain, meratakan kodenya dan membuat Sonar bahagia:

    if (sValue.endsWith("S")) {
        return new ExtractSecond(sValue).invoke();
    } // no need in else after return, code flattened

    if (sValue.endsWith("ms")) {
        return new ExtractMillisecond(sValue).invoke();
    }

    // and so on...
    return Long.parseLong(sValue); // forget millis, these aren't needed anymore

Hal lain yang patut dipertimbangkan adalah menjatuhkan blok try-catch. Ini juga akan menurunkan kompleksitas siklomatik, tetapi alasan utama mengapa saya merekomendasikannya adalah bahwa dengan blok ini, tidak ada cara bagi kode pemanggil untuk membedakan 0 yang diuraikan secara hukum dari pengecualian format angka.

Kecuali jika Anda 200% yakin bahwa mengembalikan 0 untuk kesalahan parse adalah apa yang dibutuhkan kode pemanggil, Anda lebih baik menyebarkan pengecualian itu dan membiarkan kode pemanggil memutuskan bagaimana menghadapinya. Biasanya lebih mudah untuk memutuskan di pemanggil apakah akan membatalkan eksekusi atau mencoba lagi mendapatkan input, atau kembali ke beberapa nilai default seperti 0 atau -1 atau apa pun.

Cuplikan kode Anda untuk contoh ExtractHour membuat saya merasa bahwa fungsionalitas ExtractXXX dirancang dengan cara yang jauh dari optimal. Saya bertaruh setiap kelas yang tersisa tanpa berpikir mengulangi hal yang sama parseDoubledan substring, dan mengalikan hal-hal seperti 60 dan 1000 berulang-ulang.

Ini karena Anda melewatkan esensi dari apa yang perlu dilakukan tergantung pada sValue- yaitu, ia menentukan berapa banyak karakter yang harus dipotong dari ujung string dan apa yang akan menjadi nilai pengganda. Jika Anda mendesain objek "inti" Anda di sekitar fitur-fitur penting ini, itu akan terlihat seperti berikut:

private static class ParseHelper {
    // three things you need to know to parse:
    final String source;
    final int charsToCutAtEnd;
    final long multiplier;

    ParseHelper(String source, int charsToCutAtEnd, long multiplier) {
        this.source = source == null ? "0" : source; // let's handle null here
        this.charsToCutAtEnd = charsToCutAtEnd;
        this.multiplier = multiplier;
    }

    long invoke() {
        // NOTE consider Long.parseLong instead of Double.parseDouble here
        return (long) (Double.parseDouble(cutAtEnd()) * multiplier);
    }

    private String cutAtEnd() {
        if (charsToCutAtEnd == 0) {
            return source;
        }
        // write code that cuts 'charsToCutAtEnd' from the end of the 'source'
        throw new UnsupportedOperationException();
    }
}

Setelah itu, Anda memerlukan kode yang mengkonfigurasi objek di atas per kondisi tertentu jika terpenuhi, atau entah bagaimana "memotong" sebaliknya. Ini dapat dilakukan sebagai berikut:

private ParseHelper setupIfInSecond(ParseHelper original) {
    final String sValue = original.source;
    return sValue.endsWith("s") && !sValue.endsWith("ms")
            ? new ParseHelper(sValue, 1, 1000)
            :  original; // bypass
}

private ParseHelper setupIfMillisecond(ParseHelper original) {
    final String sValue = original.source;
    return sValue.endsWith("ms")
            ? new ParseHelper(sValue, 2, 1)
            : original; // bypass
}

// and so on...

Berdasarkan blok bangunan di atas , kode metode Anda dapat terlihat sebagai berikut:

public long parseTimeValue(String sValue) {

   return setupIfSecond(
           setupIfMillisecond(
           setupIfInSecond(
           setupIfInMinute(
           setupIfHour(
           setupIfDay(
           setupIfWeek(
           new ParseHelper(sValue, 0, 1))))))))
           .invoke();
}

Anda lihat, tidak ada kerumitan yang tersisa, tidak ada kurung kurawal di dalam metode sama sekali (atau beberapa pengembalian seperti di saran kekuatan kasar asli saya pada kode perataan). Anda cukup memeriksa input secara berurutan dan menyesuaikan pemrosesan sesuai kebutuhan.

Jika Anda benar-benar ingin mengubahnya, Anda dapat melakukan sesuatu seperti ini:

// All of your Extract... classes will have to implement this interface!
public Interface TimeExtractor
{
    public long invoke();
}

private static class ExtractHour implements TimeExtractor
{
  private String sValue;


  /*Not sure what this was for - might not be necessary now
  public ExtractHour(String sValue)
  {
     this.sValue = sValue;
  }*/

  public long invoke(String s)
  {
     this.sValue = s;
     long millis;
     millis = (long) (Double.parseDouble(sValue.substring(0, sValue.length() - 1)) * 60 * 60 * 1000);
     return millis;
 }
}

private static HashMap<String, TimeExtractor> extractorMap= new HashMap<String, TimeExtractor>();

private void someInitMethod()
{
   ExtractHour eh = new ExtractorHour;
   extractorMap.add("H",eh);
   /*repeat for all extractors */
}

public static long parseTimeValue(String sValue)
{
    if (sValue == null)
    {
        return 0;
    }
    String key = extractKeyFromSValue(sValue);
    long millis;
    TimeExtractor extractor = extractorMap.get(key);
    if (extractor!=null)
    {
      try
      {
         millis= extractor.invoke(sValue);
      }
        catch (NumberFormatException e)
      {
          LOGGER.warn("Number format exception", e);
      }
    }
    else
       LOGGER.error("NO EXTRACTOR FOUND FOR "+key+", with sValue: "+sValue);

    return millis;
}

Idenya adalah Anda memiliki peta kunci (apa yang Anda gunakan di "endsWith" sepanjang waktu) yang memetakan ke objek tertentu yang melakukan pemrosesan yang Anda inginkan.

Agak kasar di sini tapi saya harap itu cukup jelas. Saya tidak mengisi rinciannya extractKeyFromSValue()karena saya hanya tidak cukup tahu apa yang harus dilakukan oleh string ini dengan benar. Sepertinya itu adalah karakter 1 atau 2 non-numerik terakhir (regex mungkin bisa ekstrak dengan cukup mudah, mungkin .*([a-zA-Z]{1,2})$akan bekerja), tapi aku tidak 100% yakin ...

Jawaban asli:

Anda bisa berubah

else if (sValue.endsWith("H") || sValue.endsWith("h")) {

untuk

else if (sValue.toUpper().endsWith("H")) {

Itu mungkin sedikit menyelamatkan Anda, tapi jujur, saya tidak akan terlalu khawatir. Saya setuju dengan Anda bahwa saya tidak berpikir ada banyak ruginya meninggalkan metode apa adanya. Alih-alih mencoba "mematuhi aturan Sonar" cobalah untuk "tetap dekat dengan pedoman Sonar, sebanyak mungkin yang wajar".

Anda dapat membuat diri Anda gila mencoba mengikuti setiap aturan tunggal yang akan dimiliki oleh alat analisis ini, tetapi Anda juga harus memutuskan apakah aturan tersebut masuk akal untuk proyek Anda, dan untuk kasus tertentu di mana waktu yang dihabiskan untuk refactoring mungkin tidak sepadan dengan itu. .

Anda dapat mempertimbangkan untuk menggunakan enum untuk menyimpan semua kasing yang tersedia dan predikat untuk nilai yang cocok. Seperti yang disebutkan sebelumnya fungsi Anda cukup mudah dibaca agar tidak berubah. Metrik-metrik itu ada untuk membantu Anda, bukan sebaliknya.

//utility class for matching values
private static class ValueMatchingPredicate implements Predicate<String>{
    private final String[] suffixes;

    public ValueMatchingPredicate(String[] suffixes) {      
        this.suffixes = suffixes;
    }

    public boolean apply(String sValue) {
        if(sValue == null) return false;

        for (String suffix : suffixes) {
            if(sValue.endsWith(suffix)) return true;
        }

        return false;
    }

    public static Predicate<String> withSuffix(String... suffixes){         
        return new ValueMatchingPredicate(suffixes);
    }       
}

//enum containing all possible options
private static enum TimeValueExtractor {                
    SECOND(
        ValueMatchingPredicate.withSuffix("S"), 
        new Function<String, Long>(){ 
            public Long apply(String sValue) {  return new ExtractSecond(sValue).invoke(); }
        }),

    MILISECOND(
        ValueMatchingPredicate.withSuffix("ms"), 
        new Function<String, Long>(){
            public Long apply(String sValue) { return new ExtractMillisecond(sValue).invoke(); }
        }),

    IN_SECOND(
        ValueMatchingPredicate.withSuffix("s"),
        new Function<String, Long>(){
            public Long apply(String sValue) { return new ExtractInSecond(sValue).invoke(); }
        }),

    IN_MINUTE(
        ValueMatchingPredicate.withSuffix("m"),
        new Function<String, Long>(){
            public Long apply(String sValue) {  return new ExtractInMinute(sValue).invoke(); }
        }),

    HOUR(
        ValueMatchingPredicate.withSuffix("H", "h"),
        new Function<String, Long>(){
            public Long apply(String sValue) {  return new ExtractHour(sValue).invoke(); }
        }),

    DAY(
        ValueMatchingPredicate.withSuffix("d"),
        new Function<String, Long>(){
            public Long apply(String sValue) {  return new ExtractDay(sValue).invoke(); }
        }),

    WEEK(
        ValueMatchingPredicate.withSuffix("w"),
        new Function<String, Long>(){
            public Long apply(String sValue) {  return new ExtractWeek(sValue).invoke(); }
        });

    private final Predicate<String>      valueMatchingRule;
    private final Function<String, Long> extractorFunction;

    public static Long DEFAULT_VALUE = 0L;

    private TimeValueExtractor(Predicate<String> valueMatchingRule, Function<String, Long> extractorFunction) {
        this.valueMatchingRule = valueMatchingRule;
        this.extractorFunction = extractorFunction;
    }

    public boolean matchesValueSuffix(String sValue){
        return this.valueMatchingRule.apply(sValue);
    }

    public Long extractTimeValue(String sValue){
        return this.extractorFunction.apply(sValue);
    }

    public static Long extract(String sValue) throws NumberFormatException{
        TimeValueExtractor[] extractors = TimeValueExtractor.values();

        for (TimeValueExtractor timeValueExtractor : extractors) {
            if(timeValueExtractor.matchesValueSuffix(sValue)){
                return timeValueExtractor.extractTimeValue(sValue);
            }
        }

        return DEFAULT_VALUE;
    }
}

//your function
public static long parseTimeValue(String sValue){
    try{
        return TimeValueExtractor.extract(sValue);
    } catch (NumberFormatException e) {
        //LOGGER.warn("Number format exception", e);
        return TimeValueExtractor.DEFAULT_VALUE;
    }
}

Terkait dengan komentar Anda tentang:

Intinya - Jangan over engineer demi Sonar (atau Manajer Proyek Setengah Baked) mengeluh tentang CC. Lakukan saja apa yang bernilai satu sen untuk proyek ini.

Pilihan lain untuk dipertimbangkan adalah mengubah standar pengkodean tim Anda untuk situasi seperti ini. Mungkin Anda dapat menambahkan semacam pemilihan tim untuk memberikan ukuran tata kelola dan menghindari situasi jalan pintas.

Tetapi mengubah standar tim dalam menanggapi situasi yang tidak masuk akal adalah tanda tim yang baik dengan sikap yang benar tentang standar. Standar ada untuk membantu tim, tidak menghalangi penulisan kode.

Sejujurnya, semua respons teknis di atas tampak sangat rumit untuk tugas yang dihadapi. Seperti yang sudah ditulis, kodenya sendiri bersih dan bagus, jadi saya akan memilih perubahan sekecil mungkin untuk memuaskan penghitung kompleksitas. Bagaimana dengan refactor berikut:

public static long parseTimeValue(String sValue) {

    if (sValue == null) {
        return 0;
    }

    try {
        return getMillis(sValue);
    } catch (NumberFormatException e) {
        LOGGER.warn("Number format exception", e);
    }

    return 0;
}

private static long getMillis(String sValue) {
    if (sValue.endsWith("S")) {
        return new ExtractSecond(sValue).invoke();
    } else if (sValue.endsWith("ms")) {
        return new ExtractMillisecond(sValue).invoke();
    } else if (sValue.endsWith("s")) {
        return new ExtractInSecond(sValue).invoke();
    } else if (sValue.endsWith("m")) {
        return new ExtractInMinute(sValue).invoke();
    } else if (sValue.endsWith("H") || sValue.endsWith("h")) {
        return new ExtractHour(sValue).invoke();
    } else if (sValue.endsWith("d")) {
        return new ExtractDay(sValue).invoke();
    } else if (sValue.endsWith("w")) {
        return new ExtractWeek(sValue).invoke();
    } else {
        return Long.parseLong(sValue);
    }
}

Jika saya menghitung dengan benar, fungsi yang diekstrak harus memiliki kompleksitas 9, yang masih melewati persyaratan. Dan itu pada dasarnya kode yang sama seperti sebelumnya , yang merupakan hal yang baik, karena kodenya bagus untuk memulai.

Plus, pembaca Clean Code mungkin menikmati kenyataan bahwa metode tingkat atas sekarang sederhana dan pendek, sedangkan yang diekstraksi membahas detail.