почему я должен рефакторинг этого кода как Cyclomatic Complexity 58 - PullRequest
5 голосов
/ 19 октября 2011

Я читал, что наличие CC 10 или менее будет очень легко обслуживаемым кодом.Но метод, который я написал, имеет CC 58. Благодаря инструменту анализа кода VS 2010.Я считаю, что метод, который я написал, очень прост, читабелен и понятен, насколько я понимаю.Следовательно, я бы не предпочел рефакторинг кода.Но так как CC выше допустимого, мне интересно, зачем один рефакторинг этого метода.Я учусь тому, как улучшить мой код. Если я ошибаюсь, пожалуйста, поправьте меня.Вот код

private string MapBathRooms(string value)
    {
        double retValue = 0;
        if (value == "1" || value == "One")
            retValue = 1;
        if (value == "OneAndHalf" || value == "1.5" || value == "1 1/2")
            retValue = 1.5;
        if (value == "2" || value == "Two")
            retValue = 2;
        if (value == "TwoAndHalf" || value == "2.5" || value == "2 1/2")
            retValue = 2.5;
        if (value == "3" || value == "Three")
            retValue = 3;
        if (value == "ThreeAndHalf" || value == "3.5" || value == "3 1/2")
            retValue = 3.5;
        if (value == "4" || value == "Four")
            retValue = 4;
        if (value == "FourAndHalf" || value == "4.5" || value == "4 1/2")
            retValue = 4.5;
        if (value == "5" || value == "Five" || value == "FourOrMore")
            retValue = 5;
        if (value == "FiveAndHalf" || value == "5.5" || value == "5 1/2")
            retValue = 5.5;
        if (value == "6" || value == "Six")
            retValue = 6;
        if (value == "SixAndHalf" || value == "6.5" || value == "6 1/2")
            retValue = 6.5;
        if (value == "7" || value == "Seven")
            retValue = 7;
        if (value == "SevenAndHalf" || value == "7.5" || value == "7 1/2")
            retValue = 7.5;
        if (value == "8" || value == "8+" || value == "Eight" || value == "SevenOrMore")
            retValue = 8;
        if (value == "EightAndHalf" || value == "8.5" || value == "8 1/2")
            retValue = 8.5;
        if (value == "9" || value == "Nine")
            retValue = 9;
        if (value == "NineAndHalf" || value == "9.5" || value == "9 1/2")
            retValue = 9.5;
        if(value == "10" || value == "Ten")
            retValue = 10;
        if (value == "TenAndHalf" || value == "10.5" || value == "10 1/2"
            || value == "10+" || value == "MoreThanTen" || value == "11")
            retValue = 10.5;

        if (retValue == 0)
            return value;

        return retValue.ToString();
    }

Ответы [ 6 ]

15 голосов
/ 19 октября 2011

Почему бы просто не иметь Dictionary<string, double>? Это сделает намного более простой код - вы отделили данные от кода поиска.

private static readonly Dictionary<string, double> BathRoomMap =
    new Dictionary<string, double>
{
    { "1", 1 },
    { "One", 1 },
    { "OneAndHalf", 1.5 },
    { "1.5", 1.5 },
    { "1 1/2", 1.5 }
    // etc
};

private static string MapBathRooms(string value)
{
    double result;
    if (!BathRoomMap.TryGetValue(value, out result))
    {
        return value; // Lookup failed
    }
    return result.ToString();
}

На самом деле, вы можете сделать это еще проще, избегая вызова ToString - просто сделайте его Dictionary<string, string>:

private static readonly Dictionary<string, string> BathRoomMap =
    new Dictionary<string, string>
{
    // Note: I've removed situations where we'd return the
    // same value anyway... no need to map "1" to "1" etc
    { "One", "1" },
    { "OneAndHalf", "1.5" },
    { "1 1/2", "1.5" }
    // etc
};

private static string MapBathRooms(string value)
{
    string result;
    if (!BathRoomMap.TryGetValue(value, out result))
    {
        return value; // Lookup failed
    }
    return result;
}

Как говорит ChrisF, вы также можете прочитать это из файла или другого ресурса.

Преимущества этого:

  • Это намного легче избежать ошибок и расширяться, ИМО. Существует простое отображение 1: 1 от входа к выходу, в отличие от логики, которая может пойти не так
  • отделяет данные от логики
  • Позволяет загружать данные из других мест, если это необходимо.
  • Поскольку инициализаторы коллекции используют Dictionary<,>.Add, если у вас есть дубликат ключа, вы получите исключение при инициализации типа, поэтому вы сразу же обнаружите ошибку.

Скажем так: вы бы когда-либо рассматривали возможность рефакторинга из версии на основе словаря в версию "много реального кода"? Я бы точно не стал.

Если вы действительно, действительно хотите иметь все это в методе, вы всегда можете использовать оператор switch:

private static string MapBathRooms(string value)
{
    switch (value)
    {
        case "One":
            return "1";
        case "OneAndHalf":
        case "1 1/2":
            return "1.5";
        ...
        default:
            return value;
    }
}

Я бы все-таки сам использовал форму словаря ... но это имеет очень небольшое преимущество, так как обнаружение дубликатов переносится на время компиляции .

3 голосов
/ 19 октября 2011

Я согласен с другими авторами относительно использования словаря для отображения, но я также хочу отметить, что подобный код часто затрудняет поиск ошибок. Например:

  • Вы конвертируете «FourOrMore» в 5, но «MoreThanTen» конвертируется в 10,5. Это кажется противоречивым.
  • Вы конвертируете «11» в 10,5, что также кажется несовместимым с остальным кодом.

Общий алгоритм для выполнения преобразования может быть сложнее написать изначально, но может легко сэкономить ваше время в долгосрочной перспективе.

0 голосов
/ 20 октября 2011

К вашему общему вопросу, для других случаев, которые не могут быть реорганизованы, как предложено другими респондентами для этой конкретной функции, существует вариант CC, который считает заявление дела как одну ветвь на том основании, что оно практически совпадает с линейные строки кода для простоты понимания (но не для тестового покрытия). Многие инструменты, которые измеряют один вариант, предложат другой. Я предлагаю использовать вариант case = 1 вместо того, который вы используете.

0 голосов
/ 19 октября 2011

С помощью принципа СУХОЙ (не повторяйтесь) вы можете заменить все эти if утверждения на switch.Переключатель будет реализован с использованием хеш-таблицы, поэтому он также будет быстрее, чем все операторы if.

Вы можете удалить все случаи, когда вы ловите числовое представление числа, так как оно обрабатываетсяотступление.

Я не вижу смысла в преобразовании строки в число, а затем снова в строку.Более эффективно использовать буквенные строки (так как они уже созданы), чем создавать строки на лету.Кроме того, это устраняет проблему культуры, когда, например, значение 9.5 может привести к строке "9,5" вместо "9.5" для некоторых культур.

private string MapBathRooms(string value) {
  switch (value) {
    case "One": value = "1"; break;
    case "OneAndHalf":
    case "1 1/2": value = "1.5"; break;
    case "Two": value = "2"; break;
    case "TwoAndHalf":
    case "2 1/2": value = "2.5"; break;
    case "Three": value = "3"; break;
    case "ThreeAndHalf":
    case "3 1/2": value = "3.5"; break;
    case "Four": value = "4"; break;
    case "FourAndHalf":
    case "4 1/2": value = "4.5"; break;
    case "Five":
    case "FourOrMore": value = "5"; break;
    case "FiveAndHalf":
    case "5 1/2": value = "5.5"; break;
    case "Six": value = "6"; break;
    case "SixAndHalf":
    case "6 1/2": value = "6.5"; break;
    case "Seven": value = "7"; break;
    case "SevenAndHalf":
    case "7 1/2": value = "7.5"; break;
    case "8+":
    case "Eight":
    case "SevenOrMore": value = "8"; break;
    case "EightAndHalf":
    case "8 1/2": value = "8.5"; break;
    case "Nine": value = "9"; break;
    case "NineAndHalf":
    case "9 1/2": value = "9.5"; break;
    case "Ten": value = "10"; break;
    case "TenAndHalf":
    case "10 1/2":
    case "10+":
    case "MoreThanTen":
    case "11": value = "10.5"; break;
  }
  return value;
}

Обратите внимание, что я оставил случайВ результате ввода "11" возвращается значение "10.5".Я не уверен, что это ошибка, но это то, что делает оригинальный код.

0 голосов
/ 19 октября 2011

Чтобы ответить почему, а не как:

Одной из причин является та, которую я упоминаю в своем комментарии к ответу Джона Скита, но использование словаря и внешнего ресурса позволяет вам изменять поведение вашего приложения безнеобходимость перестраивать его каждый раз, когда меняются требования.

Еще одна - скорость выполнения.Ваш код должен проверить несколько десятков строк, чтобы найти результат - хотя есть способы остановить выполнение, как только вы нашли совпадение, вам все равно придется потенциально проверить их все.Использование словаря даст вам линейное время доступа независимо от ввода.

0 голосов
/ 19 октября 2011

Да. Действительно очень ремонтопригодны.

Попробуйте вместо этого:

// initialize this somewhere
IDictionary<string, string> mapping;

private string MapBathRooms(string value)
{
  if (mapping.ContainsKey(value))
  {
    return mapping[value];
  }
  return value;
}

Хранение этого в словаре должно поддерживать CC равным 2. Словарь может быть инициализирован чтением его из файла или другого ресурса.

CC - это (в значительной степени) количество потенциальных путей выполнения метода. Ваш CC для этого метода настолько высок, потому что вы не использовали структуру, подходящую для решения подобных проблем (здесь: словарь). Использование соответствующих структур данных для решения проблем обеспечивает чистоту и возможность повторного использования вашего кода.

...