Гнездо против GOTO: чего лучше избегать?

В Java они на самом деле не известны как операторы GOTO и скорее называются Ветвящиеся выражения , но я обнаружил, что предыдущий термин немного больше свидетельствует о том, что они на самом деле делают.

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

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

Итак, вот моя загадка. Я надеюсь, что кто-то может вести меня и дать мне несколько советов по разумной и лучшей практике, потому что, по-видимому, мой мозг не справляется с конфликтующими сигналами.

Несколько раз через мой код у меня есть цикл for(), который выглядит следующим образом (где lines - это List<String> и values - это Map<String, String>):

for(String line : lines) {

    if(line.charAt(0) == '#') {
        LOGGER.debug("Skipping commented line in file.");
        continue;
    }

    line = line.trim().toLowerCase();
    String[] pair = line.split("=");

    if(pair.length != 2) {
        LOGGER.error("Skipping malformed line in file: " + line);
        continue;
    }

    pair[0] = pair[0].trim();
    pair[1] = pair[1].trim();

    if(values.containsKey(pair[0])) {
        LOGGER.debug("Value is already assigned.");
        continue;
    }

    values.put(pair[0], pair[1]);

    //...and so on with still more processing
}

Причина, по которой я дал немного больше в этом фрагменте кода, чем это строго необходимо для понимания варианта использования, потому что я думаю, что цикл for не очень «тугой», и что он выполняет много обработки, имеет отношение к выбору стиля. Для меня это делает код немного неаккуратным и беспорядочным, чтобы разбросать по нему несколько областей, которые возвращают читателя в начало цикла. Другими словами, он очень похож на старый добрый GOTO что все любят ненавидеть .

Таким образом, мой другой вариант дизайна заключается в том, чтобы написать его со следующим стилем:

for(String line : lines) {
    if(!line.charAt(0) == '#') {
        line = line.trim().toLowerCase();
        String[] pair = line.split("=");
        if(pair.length == 2) {
            pair[0] = pair[0].trim();
            pair[1] = pair[1].trim();
            if(!values.containsKey(pair[0])) {
                values.put(pair[0], pair[1]);
                //...and so on with still more processing
            }
            else {
                LOGGER.debug("Value is already assigned.");
            }
        }
        else {
            LOGGER.error("Skipping malformed line in file: " + line);
        }
    }
    else {
        LOGGER.debug("Skipping commented line in file.");
    }
}

Оба эти метода /стилей заставляют мой глаз дрожать по разным причинам, и я продолжаю переписывать свой код, чтобы он соответствовал тому, какая рационализация выигрывает в текущий момент. Я также думаю, что если вы окончательно выберите один из них как «Стиль», с которым всегда нужно идти, тривиально расширять приведенные выше фрагменты кода до такой степени, что имеет смысл идти с другим.

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

71 голос | спросил asteri 27 Jpm1000000pmMon, 27 Jan 2014 19:03:59 +040014 2014, 19:03:59

11 ответов


57

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

public final class Pair {
    private String key;
    private String value;

    public Pair(key, value) {
        // TODO: Add proper argument checking/throw exceptions.

        this.key = key;
        this.value = value;
    }

    public String getKey() {
        return key;
    }

    public String getValue() [
        return value;
    }

    /**
     * Returns the string representation which looks like @{code key=value}.
     */
    @Override
    public String toString() {
        return key + "=" + value;
    }
}

И статический заводский метод:

/**
 * Constructs a Pair from the given string,
 * returns null if the string is misformed or
 * the string was null.
 * 
 * A well formed input looks like {@code key=value} and
 * does not start with a {@code #} as that indicates comments.
 */
public static Pair fromString(String str) {
    // We do not accept nulls and empty strings.
    if (str == null || str.empty()) {
        return null;
    }

    // TODO: Figure out if trimming of str would be a good idea.

    // Skip comments.
    if (str.startsWith("#")) {
        return null;
    }

    String[] splitted = str.split("=");

    // We also do not accept anything else.
    if (splitted.length != 2) {
        return null;
    }

    // TODO: Are you accepting zero-length keys/values?

    return new Pair(splitted[0], splitted[1]);
}

В основном он выглядит так же, как ваш цикл, с двумя важными отличиями:

  1. Это собственный метод, легко повторяемый, легко проверяемый.
  2. У него есть документация, как должен выглядеть вход.

Теперь к вашему циклу:

for (String line : lines) {
    Pair pair = Pair.fromString(line);
    if (pair != null && !values.containsKey(pair.getKey())) {
        // TODO
    }
}

Это не только сокращает код внутри цикла, но и в конце делает его более читаемым, потому что теперь вы используете pair.getValue() вместо пары pair[1].

Исходя из контекста этого, я бы предложил другие имена, например StringPair, Setting, SettingsPair или ConfigEntry

Вот ваш цикл с операторами журнала (при условии, что LOGGER имеет перегрузку, которая похожа на Logger.log(...)):

for (String line : lines) {
    Pair pair = Pair.fromString(line);
    if (pair != null) {
        if (!values.containsKey(pair.getKey()) {
            // TODO
        } else {
            LOGGER.debug("Pair is already assigned: {0}", pair);
        }
    } else {
        LOGGER.error("Skipping line: {0}", line);
    }
}

Нет никаких мелких сообщений об ошибках, потому что из ввода должно быть очевидно, почему функция fromString не может создать Pair.

Хотя мы вернулись к двум if -statemens с ветвями else, читаемость намного лучше.


Это нарушает пункт 43 из Effective Java (2nd Edition), который выглядит следующим образом:

  

Возвращает пустые массивы или коллекции, а не нули

Но почему я предложил такой подход? Эффективная Java заявляет, что лучше всего выделять мелкозернистые исключения, а не возвращать null, чтобы облегчить работу с этим методом. Это справедливо для любого метода, но это не то, что должен делать этот подход. Мы не заботимся о том, какая проблема существует, мы просто хотим, чтобы объект, который можно было использовать для нашей программной логики или двигаться дальше, отбрасывал мелкозернистые ошибки в процессе.

Если вы используете этот подход, вам нужно запомнить некоторые вещи:

  • Не используйте конструктор для этой цели, но статический заводский метод.
  • Полностью документируйте поведение и цель статического заводского метода.
  • Это должно быть очевидно при просмотре ввода, возвращаемого функцией.
  • Знайте, когда использовать это и когда использовать обработку исключений (помните, почему это не универсальное решение «все-в-одном» для всего).
  • Если это внутри библиотеки /общедоступного API, всегда предоставляйте способ, который генерирует мелкозернистые исключения.

В случае, если вы обрабатываете файл ini или настроек, который не требует специального обращения, ответ от AJMansfield правильно .

  

Не сворачивайте свою версию, если для нее работает java.util.Properties.

ответил Bobby 27 Jpm1000000pmMon, 27 Jan 2014 19:49:59 +040014 2014, 19:49:59
46

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

Рассмотрим , почему инструкция goto считается вредным . Поскольку он имеет потенциал для перехода в произвольные места в коде, он может превратить вашу программу в кучу спагетти.

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

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

ответил 200_success 27 Jpm1000000pmMon, 27 Jan 2014 19:21:19 +040014 2014, 19:21:19
23

Мне кажется странным, что проблема линии нулевой длины еще не указана:

for(String line : lines) {

     if(line.charAt(0) == '#') {
         ....

В приведенном выше коде предполагается непустая строка во всех случаях, иначе вы получите IndexOutOfBoundsException .

Это проблема, которая должна быть идентифицирована и покрыта тестированием модулей.

Это также приводит к тому, что вы являетесь , и следует рассмотреть предложение использовать java.util.Properties .

Несмотря на это предложение, я должен взвесить на теоретическом обсуждении continue /break. Точки входа и выхода петли уже являются целями инструкций ветвления в скомпилированном (JIT или байте) коде, который производит Java. Эти точки ветвления хорошо управляются и имеют чистые и хорошо документированные условия входа /выхода. Они не похожи на концепцию GOTO. Со временем у меня стало очень удобно использовать их, и они не более подозрительны, чем ранний возврат из вызова метода. Фактически, концептуально это то, что они есть, просто раннее завершение блока кода, который находится в цикле, и завершение может разрешить (continue) или disallow (break).

Я бы не рекомендовал использовать эти утверждения безнаказанно, но тот факт, что break является естественным синтаксисом в инструкции switch, дает некоторый намек на то, что это нормально использовать. Это правильный инструмент для некоторых работ. Используйте его, если это необходимо.

ответил rolfl 28 Jam1000000amTue, 28 Jan 2014 04:51:59 +040014 2014, 04:51:59
23

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

Вот пример.

enum Validator {
    IsNotComment("Skipping commented line in file.") {

        @Override
        boolean isValid(String s) {
            return !s.startsWith("#");
        }

    },
    IsAPair("Not a pair") {

        @Override
        boolean isValid(String s) {
            return s.trim().split("=").length == 2;
        }

    };

    abstract boolean isValid(String s);

    public final String failMsg;

    Validator(String failMsg) {
        this.failMsg = failMsg;
    }
}

public void test() {
    String[] lines = {"Hello"};
    Map<String,String> values = new HashMap<>();
    for (String line : lines) {
        boolean valid = true;
        String failMsg = "";
        for ( Validator v : Validator.values() ) {
            valid &= v.isValid(line);
            if ( !valid ) {
                failMsg = v.failMsg;
            }
        }
        if ( valid ) {
            // Do stuff with valid lines.
            String [] parts = line.trim().split("=");
            values.add(parts[0],parts[1]);
        } else {
            System.out.println("Failed validation: "+failMsg);
        }
    }
}

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

Это побуждает вас сосредоточиться на том, что вы пытаетесь смешивать проверку с синтаксическим разбором.

Если вы позволите своим заботам правильно следовать вам, вы увидите, что проверка является отдельным процессом, и ваша попытка смешать проверку в процессе синтаксического анализа делает ваш запах кода.

Проверить сначала - затем разобрать. Сделайте не предварительную оптимизацию и объедините их в один, пока у вас не будет хорошая измеренная причина, чтобы сделать это - и даже затем снова измерить.

Добавлено

Просто для радости этого - вот как это могло бы выглядеть в Java 8

List<String> lines = Arrays.asList(
        "# Comment",
        "",
        "A=B",
        "A=C",
        "X=Y ",
        "P = Q");

public void test() {
    Map<String,String> values = new HashMap<>();
    lines.stream()
            // Trim it and lowercase.
            .map(s -> s.trim().toLowerCase())
            // Discard empty lines - good call @rolfl
            .filter(s -> !s.isEmpty())
            // Discard comments.
            .filter(s -> s.charAt(0) != '#')
            // Split the trimmed form
            .map(s -> s.split("="))
            // Must be two parts.
            .filter(a -> a.length == 2)
            // Trim each one.
            .map(a -> new String[]{a[0].trim(), a[1].trim()})
            // Not seen already.
            .filter(a -> !values.containsKey(a[0]))
            // Install in values.
            .forEach (a -> values.put(a[0], a[1]));
    System.out.println(values);
}

Это действительно действительно печатает:

{p=q, a=b, x=y}

Теперь это круто или что?

Извините, но это не делает всю полезную печать и прочее.

ответил OldCurmudgeon 28 Jam1000000amTue, 28 Jan 2014 03:46:38 +040014 2014, 03:46:38
18

Вот далеко не лучшее решение: используйте java.util.Properties для обработки вашего файла конфигурации. Properties поддерживает key=value , который очень похож на тот, который вы пытаетесь проанализировать. Вам решать, достаточно ли синтаксис для работы с любыми существующими файлами конфигурации, которые могут иметься, и важны ли проверка и ведение журнала в вашем текущем коде.

ответил AJMansfield 27 Jpm1000000pmMon, 27 Jan 2014 21:27:15 +040014 2014, 21:27:15
10

Я согласен с @ 200_sucess, что первая версия лучше.

Альтернативой может быть подпрограмма:

for(String line : lines) {
    AddLine(line, values);
    //...and so on with still more processing
}

void AddLine(String line, Map<String, String> values)
{
    if(line.charAt(0) == '#') {
        LOGGER.debug("Skipping commented line in file.");
        return;
    }

    line = line.trim().toLowerCase();
    String[] pair = line.split("=");

    if(pair.length != 2) {
        LOGGER.error("Skipping malformed line in file: " + line);
        return;
    }

    pair[0] = pair[0].trim();
    pair[1] = pair[1].trim();

    if(values.containsKey(pair[0])) {
        LOGGER.debug("Value is already assigned.");
        return;
    }

    values.put(pair[0], pair[1]);
}

Не возражаете ли вы «раннее возвращение» в подпрограмме? Вы закончили бы подпрограмму, чтобы вернуться или построить глубоко-вложенную, если?

ответил ChrisW 27 Jpm1000000pmMon, 27 Jan 2014 19:29:47 +040014 2014, 19:29:47
8

Вот ответ из левого поля, но это может дать вам некоторое вдохновение. Если бы я делал это в Haskell, я бы написал его как конвейер карт и сгибов в списке. Я не очень разбираюсь в Java, но можете ли вы переписать свою функцию как несколько более мелких функций, каждый из которых действует на итераторе и создает новый итератор для подачи следующей функции?

processLines = insertInto dict
              . map trimPairs
              . filter notMalformed
              . map splitLine
              . filter notComment
ответил Tom Ellis 28 Jpm1000000pmTue, 28 Jan 2014 12:27:04 +040014 2014, 12:27:04
6

Позвольте мне ответить @AJMasfield, что для этого конкретного примера Не переустанавливать колесо. . Однако этот вопрос возникает в контексте, отличном от свойств. Во второй версии сообщения LOGGER все дальше и дальше удаляются от соответствующих предложений if. В первой версии используется идиома, с которой мы должны быть знакомы, проверяя предварительные условия и выходы ошибок вверху. Я считаю это вполне читаемым.

ответил Andrew Lazarus 28 Jam1000000amTue, 28 Jan 2014 02:23:56 +040014 2014, 02:23:56
5

Решение вашей проблемы (приведено в примере):

  1. Выберите содержимое блока for
  2. Рефактор -> Метод извлечения
  3. Проверьте программу, чтобы убедиться, что ничего не сломано
  4. Осмотрите новый сгенерированный метод, чтобы убедиться, что он уродливый
  5. Если уродливые, выберите уродливые части, перейдите к # 2
  6. Если вы не можете выбрать их, потому что они слишком разбросаны по телу, перегруппируйте, пока не сможете
  7. Если вы не можете изменить порядок, потому что код сломается, передумайте дизайн вашего алгоритма - действительно ли это должно быть запутанным?
  8. Если да, примите его и перейдите. Не весь код может быть красивым, а иногда и перфекционизм не стоит.

Ответ на ваш вопрос (пример в сторону):

goto очень редко требуется на современных языках. Отчасти причина в том, что существуют такие вещи, как return, break, continue и throw). Это в основном очень ограниченные версии goto, которые неспособны вызвать где-то почти столько же хаоса, но все равно могут удовлетворить популярный вариант использования goto. Вряд ли вы столкнетесь с достойным кодом goto, который нельзя приручить с помощью этих инструментов и применить некоторые интеллектуальные рефакторинг.

ответил Superbest 28 Jam1000000amTue, 28 Jan 2014 09:42:04 +040014 2014, 09:42:04
3

Я согласен с другими, что первый из них лучше, потому что его легче читать, но @AJMansfield также имеет очень хорошую точку. Стоит отметить, что вы также можете использовать > Splitter и MapSplitter или написать что-то подобное.

ответил palacsint 28 Jam1000000amTue, 28 Jan 2014 01:33:57 +040014 2014, 01:33:57
0

Основываясь на понятии «просить прощения, а не разрешения», я бы поставил обработку строки в блок try-catch, например:

for(String line : lines) {

    try {
        if(line.charAt(0) == '#') {
            throw new Exception("Skipping commented line in file.");
        }

        line = line.trim().toLowerCase();
        String[] pair = line.split("=");

        pair[0] = pair[0].trim();
        pair[1] = pair[1].trim();

        if(values.containsKey(pair[0])) {
            throw new Exception("Value is already assigned.");
        }

        values.put(pair[0], pair[1]);

        //...and so on with still more processing
    } catch (Exception e) {
        LOGGER.error("Skipping line in file: " + line + " " + e.getMessage());
    }
}

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

Я бы, вероятно, извлек метод для включения values.put с тегом containsKey, который выкинул бы исключение. Любой тип теста, который необходимо выполнить, можно извлечь методу, который генерирует исключение, если условие не выполняется, например

public void testComment(String line) throws Exception {
    if (line.charAt(0)) {
        throw new Exception("Skipping commented line in file.");
    }
}

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

ответил njzk2 30 Jpm1000000pmThu, 30 Jan 2014 18:08:06 +040014 2014, 18:08:06

Похожие вопросы

Популярные теги

security × 330linux × 316macos × 2827 × 268performance × 244command-line × 241sql-server × 235joomla-3.x × 222java × 189c++ × 186windows × 180cisco × 168bash × 158c# × 142gmail × 139arduino-uno × 139javascript × 134ssh × 133seo × 132mysql × 132