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

Другой способ состоит в том, чтобы эти пары были представлены классом, который сам имеет статический заводский метод, который будет возвращать 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.

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

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

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

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

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

for(String line : lines) {

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

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

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

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

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

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

Ваша проблема связана не с конструкцией цикла, а с 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}

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

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

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

Я согласен с @ 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]);
}

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

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

processLines = insertInto dict
              . map trimPairs
              . filter notMalformed
              . map splitLine
              . filter notComment

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

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

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

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

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

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

Основываясь на понятии «просить прощения, а не разрешения», я бы поставил обработку строки в блок 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.");
    }
}

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