goto

[ds.Dante]

Сегодня я переписал одну чужую функцию, добавив туда goto, и внезапно понял, что использую этот оператор в среднем раз в 3 месяца. В качестве своего оправдания приведу "до" и "после" :)

public static bool GetFree (String Expression, IEnumerable<String> Exists, out String Result)
{
	Result = Expression;

	bool UseParam = false;
	int Param = 0;
	NumberType Type = NumberType.Any;

	Match Match = MR.Match (Expression);
	if (!Match.Success)
		return false;

	Group G = Match.Groups["sOpt"];
	if (G.Captures.Count != 1)
		return false;

	String Tmp = G.Value;
	if (!String.IsNullOrEmpty (Tmp))
	{
		if (!Tmp.StartsWith ("(") || !Tmp.EndsWith (")"))
			return false;
						
		Tmp = Tmp.Substring (1, Tmp.Length - 2);
		if (!String.IsNullOrEmpty (Tmp))
		{
			if (Tmp.StartsWith (NumberType.Any.ToString ()))
			{
				#region Any
				Tmp = Tmp.Substring (3);
				Type = NumberType.Any;
				if (!String.IsNullOrEmpty (Tmp))
				{
					if (!Tmp.StartsWith ("(") || !Tmp.EndsWith (")"))
						return false;

					Tmp = Tmp.Substring (1, Tmp.Length - 2);
					if (!String.IsNullOrEmpty (Tmp))
					{
						int p;
						if (!int.TryParse (Tmp, out p))
							return false;

						Param = p;
						UseParam = true;
					}
				}
				#endregion
			}
			else if (Tmp.StartsWith (NumberType.Next.ToString ()))
			{
				#region Next
				Tmp = Tmp.Substring (4);
				Type = NumberType.Next;
				if (!String.IsNullOrEmpty (Tmp))
				{
					if (!Tmp.StartsWith ("(") || !Tmp.EndsWith (")"))
						return false;
					Tmp = Tmp.Substring (1, Tmp.Length - 2);
					if (!String.IsNullOrEmpty (Tmp))
					{
						int p;
						if (!int.TryParse (Tmp, out p))
							return false;

						Param = p;
						UseParam = true;
					}
				}
				#endregion
			}
			else
				return false;
		}
	}

	return GetFreeNumber (Expression.Replace (Match.Value, String.Empty), Exists, Match.Index, Type, Param, UseParam, out Result);
}

Вот так стало:

public static bool GetFree (String Expression, IEnumerable<String> Exists, out String Result)
{
	bool useParam = false;
	int startNumber = 0;
	NumberType type = NumberType.Any;
	Result = Expression;

	Match match = MR.Match (Expression);
	if (!match.Success)
		return false;
	Group group = match.Groups["sOpt"];
	if (group.Captures.Count != 1)
		return false;

	String selected = group.Value;
	if (String.IsNullOrEmpty (selected))
		goto _return;
	if (!selected.StartsWith ("(") || !selected.EndsWith (")"))
		return false;

	selected = selected.Substring (1, selected.Length - 2);
	if (String.IsNullOrEmpty (selected))
		goto _return;

	if (selected.StartsWith (NumberType.Any.ToString ()))
		type = NumberType.Any;
	else if (selected.StartsWith (NumberType.Next.ToString ()))
		type = NumberType.Next;
	else
		return false;

	selected = selected.Substring (type.ToString().Length);
	if (String.IsNullOrEmpty (selected))
		goto _return;
	if (!selected.StartsWith ("(") || !selected.EndsWith (")"))
		return false;

	selected = selected.Substring (1, selected.Length - 2);
	if (String.IsNullOrEmpty (selected))
		goto _return;
	if (!int.TryParse (selected, out startNumber))
		return false;
	useParam = true;

_return:
	return GetFreeNumber (Expression.Replace (match.Value, String.Empty), Exists, match.Index, type, startNumber, useParam, out Result);
}

Тут подошёл бы вариант из "Совершенного кода":

do
{
	...
	if (hasResult)
		break; // вместо goto
	...
}
while (false);

Но это - лишь формальный способ избежать goto ценой большей вложенности и нечитабельности.

[Utkin]

Я не особо всматривался в Ваш код, но ИМХО есть еще варианты. Вы просматриваете оба случая и когда false и когда _return. Можно попытаться перестроить код так чтобы рассматривать только одну ветвь поиска - например только false. Если false Не выполнится тогда однозначно _return. Но это только взгляд на поверхности (не вдаваясь в суть алгоритма).

[Скарам]

ds.Dante, не вчитываясь в код могу сказать, что использование goto можно заменить retur-ом, не шагать к _return, а сразу делать возврат, можно написать метод, обрабатывающий данные перед возвратом. Лучше такую кракозябру разбить на меньшие функции, с осмысленными названиями... способов море, не всегда все запихнуть в одну функцию-лучгее решение.

[ds.Dante]

Сообщение от Utkin

Вы просматриваете оба случая и когда false и когда _return. Можно попытаться перестроить код так чтобы рассматривать только одну ветвь поиска - например только false. Если false Не выполнится тогда однозначно _return.

По мере того, как я парсю строку, в любой момент может возникнуть ситуация, либо что дальше парсить нельзя - тогда false, либо что парс окончен - тогда _return.

Сообщение от Скарам

использование goto можно заменить retur-ом, не шагать к _return, а сразу делать возврат

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

Сообщение от Скарам

можно написать метод, обрабатывающий данные перед возвратом.

И этому методу придётся передавать все те же самые локальные переменные. Никакой выгоды, только усложнение.

Напоминает шахматы: "- а если... -а тогда... -а если... -а тогда..." :)

Сообщение от Скарам

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

Вообще-то функция получилась небольшая и цельная. Можно даже объединить с GetFreeNumber, который нигде больше не вызывается.

[Utkin]

По мере того, как я парсю строку, в любой момент может возникнуть ситуация, либо что дальше парсить нельзя - тогда false, либо что парс окончен - тогда _return.

Не увидел где парсить нельзя... Парсить по-моему можно, иное дело что результаты будут некорректными, но мы об этом и так вроде знаем.... Если боитесь неоднозначностей - введите дополнительный флаг, говорящий о ложности вычислений.
Опишите задачу подробней - Вы разбираете строку? Вкратце алгоритм. Возможно просто стоит его пересмотреть.

[pu4koff]

Работает и ладно. В целом код фиговый. Получился какой-то автомат состояний, но реализованный через одно место. Короче, я бы не трогал, если это работает

[ds.Dante]

Строка может любого вида из списка:

qwe{?Number}.txt
qwe{?Number()}.txt
qwe{?Number(Any)}.txt
qwe{?Number(Any())}.txt
qwe{?Number(Any(0))}.txt
qwe{?Number(Next)}.txt
qwe{?Number(Next())}.txt
qwe{?Number(Next(1))}.txt

Нужно вычленить начало ("qwe"), конец (".txt"), слово "Any" или "Next" в теге, и число в скобках. Если строка с ошибкой, возвращаем false.

Алгоритм я трогать не стал, просто добился читабельности кода и исправил кое-какие косяки. Суть в том, что последовательно "отгрызаем" с краёв отпарсенную часть.

Сообщение от pu4koff

Работает и ладно. В целом код фиговый. Получился какой-то автомат состояний, но реализованный через одно место. Короче, я бы не трогал, если это работает

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

[Utkin]

Наверняка можно было бы регексы заюзать, просто я в них пока вообще не разбираюсь.

Оптимальный вариант.
Вариант №2 Пытаться разбить на элементы последную строку (та что qwe{?Number(Next(1))}.txt ) и уже в обычном foreach (!) проверять корректность элементов (на соответствие шаблону). Без goto и наглядно.

[ds.Dante]

Сообщение от Utkin

Вариант №2 Пытаться разбить на элементы последную строку (та что qwe{?Number(Next(1))}.txt ) и уже в обычном foreach (!) проверять корректность элементов (на соответствие шаблону). Без goto и наглядно.

Не прокатит со строкой "qwe{?Number(Any(0((}.txt" (нужно выдать ошибку) или "qwe{?Number}.txt" (другое число скобок).

[Utkin]

Все прокатит . Вот и выяснили, что дело не в конкретной функции, а в некоторых пробелах ТС касательно парсинга строк. Уже корректность расстановок скобок - элементарная операция и пишется на раз-два.
Да, это мой вариант будет длинней и состоять из нескольких функций, но по-моему скромному опыту искать ошибку, а тем более править код будет намного проще. но дело естественно за Вами...
Да и читая Совершенный код, Вы должны были бы заметить пожелания автора книги разбивать сложные и наверченные функции на более элементарные...

ЗЫ. Для получения элементов вида скобки в скобках в скобках и т.д. отлично подходит рекурсия.