Гнездо против 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.");
}
}
Оба эти метода /стилей заставляют мой глаз дрожать по разным причинам, и я продолжаю переписывать свой код, чтобы он соответствовал тому, какая рационализация выигрывает в текущий момент. Я также думаю, что если вы окончательно выберите один из них как «Стиль», с которым всегда нужно идти, тривиально расширять приведенные выше фрагменты кода до такой степени, что имеет смысл идти с другим.
Есть ли у кого-то более четкое изложение того, когда использовать один стиль над другим? Или даже некоторые хорошие аргументы, чтобы обратиться к ситуации, чтобы убедиться, что вы следуете за правилом и правилами кодирования? Любые общие советы, рекомендации и советы для решения подобных ситуаций?
11 ответов
Другой способ состоит в том, чтобы эти пары были представлены классом, который сам имеет статический заводский метод, который будет возвращать 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]);
}
В основном он выглядит так же, как ваш цикл, с двумя важными отличиями:
- Это собственный метод, легко повторяемый, легко проверяемый.
- У него есть документация, как должен выглядеть вход.
Теперь к вашему циклу:
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
. В первой версии используется идиома, с которой мы должны быть знакомы, проверяя предварительные условия и выходы ошибок вверху. Я считаю это вполне читаемым.
Решение вашей проблемы (приведено в примере):
- Выберите содержимое блока
for
- Рефактор -> Метод извлечения
- Проверьте программу, чтобы убедиться, что ничего не сломано
- Осмотрите новый сгенерированный метод, чтобы убедиться, что он уродливый
- Если уродливые, выберите уродливые части, перейдите к # 2
- Если вы не можете выбрать их, потому что они слишком разбросаны по телу, перегруппируйте, пока не сможете
- Если вы не можете изменить порядок, потому что код сломается, передумайте дизайн вашего алгоритма - действительно ли это должно быть запутанным?
- Если да, примите его и перейдите. Не весь код может быть красивым, а иногда и перфекционизм не стоит.
Ответ на ваш вопрос (пример в сторону):
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.");
}
}
В заключение я считаю, что исключения работают в этом случае, потому что вы ожидаете определенного ввода, и все, что не соответствует ему (любое исключение), оно должно быть отброшено.