gifts2017

Мастер-класс от Poppy (практикум по рефакторингу)

Опубликовал Poppy (poppy) в раздел Программирование - Практика программирования

Попался на глаза код неопытного программиста. Не смогла пройти мимо, захотелось выполнить рефактроинг. Вот что из этого получилось.
В качестве подопытного кода была выбрана статья http://infostart.ru/blogs/688/. Не буду обсуждать полезность и нужность рассматриваемого кода, просто переставлю строчки кода так, что бы с ними можно было работать. Смотрим результат:

Функция ОбойтиМетаданные(П, Код)
	//Обходит метаданные, выбирая код отбора...
	ТЗ=СоздатьКолонкиТЗРезультатов();
	
	П.Вставить("ТЗ", ТЗ);
	П.Вставить("Пропустить", ложь); 
	
	Описание=Новый Структура();
	Описание.Вставить("Класс", "Константы");
	Описание.Вставить("Вид", Неопределено);
	Описание.Вставить("ТЧ", Неопределено);
	
	ОбойтиАтрибуты(Метаданные.Константы, "Реквизит", П, Описание, Код);

	СобратьДанныеПоМетаданному(П, Код, "Справочники", 				истина, ложь,   истина);
	СобратьДанныеПоМетаданному(П, Код, "Документы", 				истина, ложь,   истина);
	СобратьДанныеПоМетаданному(П, Код, "ЖурналыДокументов", 		ложь,   ложь,   ложь);
	СобратьДанныеПоМетаданному(П, Код, "Перечисления", 				ложь,   ложь,   ложь);
	СобратьДанныеПоМетаданному(П, Код, "Отчеты", 					истина, ложь,   истина);
	СобратьДанныеПоМетаданному(П, Код, "Обработки", 				истина, ложь,   истина);
	СобратьДанныеПоМетаданному(П, Код, "ПланыВидовХарактеристик", 	истина, ложь,   истина);
	СобратьДанныеПоМетаданному(П, Код, "ПланыСчетов", 				истина, ложь,   истина);
	СобратьДанныеПоМетаданному(П, Код, "ПланыВидовРасчета", 		истина, ложь,   истина);
	СобратьДанныеПоМетаданному(П, Код, "РегистрыСведений", 			истина, истина, ложь);
	СобратьДанныеПоМетаданному(П, Код, "РегистрыНакопления", 		истина, истина, ложь);
	СобратьДанныеПоМетаданному(П, Код, "РегистрыБухгалтерии", 		истина, истина, ложь);
	СобратьДанныеПоМетаданному(П, Код, "РегистрыРасчета", 			истина, истина, ложь);
	СобратьДанныеПоМетаданному(П, Код, "БизнесПроцессы", 			истина, ложь,   истина);
	СобратьДанныеПоМетаданному(П, Код, "Задачи", 					истина, ложь,   истина);
	
	Возврат П;
КонецФункции

Процедура СобратьДанныеПоМетаданному(П, Код, Класс, ЕстьРеквизиты = ложь, ЕстьИзмеренияИРесурсы = ложь, ЕстьТЧ = ложь)
	
	Описание=Новый Структура();
	Описание.Вставить("Класс", Класс);
	Описание.Вставить("Уровень", "Класс");
	ОбойтиМетаданныеСлуж(П, Описание, Код);
	Если П.Пропустить Тогда
		Возврат;
	КонецЕсли;
	
	Для Каждого МДВид ИЗ Метаданные[Класс] Цикл
		Описание.Вставить("Вид", МДВид.Имя);
		Описание.Вставить("ТЧ", Неопределено);
		Описание.Вставить("Уровень", "Вид");
		Описание.Вставить("Синоним", МДВид.Синоним);
		ОбойтиМетаданныеСлуж(П, Описание, Код);
		Если П.Пропустить Тогда
			Продолжить;
		КонецЕсли;
		
		Если ЕстьРеквизиты Тогда
			ОбойтиАтрибуты(МДВид.Реквизиты, "Реквизит", П, Описание, Код);
		КонецЕсли;
		
		Если ЕстьИзмеренияИРесурсы Тогда
			ОбойтиАтрибуты(МДВид.Измерения, "Измерение", П, Описание, Код);
			ОбойтиАтрибуты(МДВид.Ресурсы, "Ресурс", П, Описание, Код);
		КонецЕсли;
		
		Если ЕстьТЧ Тогда
			Для Каждого МД_ТЧ ИЗ МДВид.ТабличныеЧасти Цикл
				Описание.Вставить("ТЧ", МД_ТЧ.Имя);
				ОбойтиАтрибуты(МД_ТЧ.Реквизиты, "Реквизит", П, Описание, Код);
			КонецЦикла;
		КонецЕсли;
	КонецЦикла;
КонецПроцедуры

Функция СоздатьКолонкиТЗРезультатов()
	ТЗ=Новый ТаблицаЗначений();
	ТЗ.Колонки.Добавить("Класс");
	ТЗ.Колонки.Добавить("Вид");
	ТЗ.Колонки.Добавить("ТЧ");
	ТЗ.Колонки.Добавить("Атрибут");
	ТЗ.Колонки.Добавить("ТипАтрибута");
	ТЗ.Колонки.Добавить("ВидАтрибута");
	ТЗ.Колонки.Добавить("Синоним");
	ТЗ.Колонки.Добавить("Уровень");
	
	Возврат ТЗ;
КонецФункции

Процедура ОбойтиАтрибуты(Атрибуты, ВидАтрибута, П, Описание, Код)
	Для Каждого МДАтрибут ИЗ Атрибуты Цикл
		Описание.Вставить("Уровень", "Атрибут");
		Описание.Вставить("Атрибут", МДАтрибут.Имя);
		Описание.Вставить("Синоним", МДАтрибут.Синоним);
		Описание.Вставить("ТипАтрибута", МДАтрибут.Тип);
		Описание.Вставить("ВидАтрибута", ВидАтрибута);
		ОбойтиМетаданныеСлуж(П, Описание, Код);
	КонецЦикла;
КонецПроцедуры

Функция ОбойтиМетаданныеСлуж(П, Описание, Код)
	//___обксОбработкаПрерыванияПользователя();
	П.Пропустить = ложь;
	Р = ложь;
	Выполнить(Код);
	Если Р = истина Тогда
		Стр = П.ТЗ.Добавить();
		ЗаполнитьЗначенияСвойств(Стр, Описание);
	КонецЕсли;
	Возврат Р;
КонецФункции


Кроме выполненного рефакторинга, еще и расширена функциональность. Теперь можно получать список не только реквизитов объектов, но и реквизитов табличных частей объектов.
Про рефакторинг написано здесь http://infostart.ru/blogs/652/
Рефакторинг выполнен не полностью. Опущен важный метод - Переименование. Так сказать, для сохранения исходной самобытности.

См. также

Подписаться Добавить вознаграждение
Комментарии
1. fixin 04.11.08 15:10
можно вкратце объяснить суть рефакторинга?
2. Вадим 1С911.BY (Вадимко) 04.11.08 16:52
Оба кода являются наикрасивейшими образчиками мЫшления!
3. Сhe Burashka (CheBurator) 04.11.08 17:13
У Поппы - понятнее.. вот и весь рефакторинг (упрощенно)
4. fixin 04.11.08 17:22
(3) Ну если так понятнее Попе, это его дело. ;-) Я не хотел дробить эту функцию на несколько, т.к. постоянно приходится ее тянуть в разные обработки.
За необоснованное использование тега "неопытный кодер" минусану. ;-)
AlexO; Uscolegy; +2 7 Ответить
5. BabySG (BabySG) 04.11.08 17:39
Совершенно обоснованное мнение по "неопытному кодеру" - повторять н-раз один и тот же код - признак неопытности.
Так что Poppy совершенно права и её код читается намного легче.

А отмазки, типа: "Я не хотел дробить эту функцию на несколько, т.к. постоянно приходится ее тянуть в разные обработки." выглядят вообще смешно.
Зато прикольно будет, наверное, когда прицип обода поменяется и нужно будет изменять код в Н-местах вместо одного.
6. Алексей Константинов (alexk-is) 04.11.08 19:04
Рефакторинг - это не только перестановка строк местами и вынос повторов в процедуры и функции...
Стало "понятнее" - ну, не намного. Код по-прежнему не читается с листа. Переменные, процедуры и функции не говорят сами за себя. "СоздатьМДКласс" - на самом деле создает массив с обрабатываемыми видами объектов метаданных. И так почти по каждой строке... (без бутылки не разберешься :) ).
А вот что медленнее работать будет это точно. :(
И не всегда минимум кода это хорошо.

Например:
Вариант 1
Код
Массив = Новый Массив(7);
Массив[0] = 1;
Массив[1] = 2;
Массив[2] = 3;
Массив[3] = 4;
Массив[4] = 5;
Массив[5] = 6;
Массив[6] = 7;
Показать полностью

Вариант 2
Код
Массив = Новый Массив(7);
Для Индекс = 0 По 6 Цикл
    Массив[Индекс] = Индекс + 1;
КонецЦикла;
Показать полностью

Сравним:
Вариант 1 - 8 строк кода
Вариант 2 - 4 строки кода
Вариант 1 - 8 команд при выполнении
Вариант 2 - 26 команд при выполнении
Кроме этого вариант 2 работает в 6-10 раз медленее...

Обратите внимание на 175 пост http://infostart.ru/blogs/652/ кратко и по существу...
mailwood; igor_gk; +2 Ответить
7. Lomok (lomok) 05.11.08 08:49
2 alexk-is
Вариант 1 - 8 команд при выполнении
Вариант 2 - 26 команд при выполнении
Кроме этого вариант 2 работает в 6-10 раз медленее...

Я все понимаю, но это ж не ассемблер :)
"Медленнее" в данном случае весьма условно.
К тому же, а если бы массив содержал 100 элементов?
8. quest (quest) 05.11.08 10:04
ИМХО, рефакторинг требует юниттестирования. что бы быть уверенным что отрефакторил правильно. А здесь оного не увидел.
artbear; fez; +2 Ответить 1
9. Альтаир (Altair777) 05.11.08 10:12
(7) > К тому же, а если бы массив содержал 100 элементов?
Мне кажется, что на практике очень редко массивы заполняются значениями по простым формулам, зависящим от индекса элемента
И, опять же - рефакторинг - это не догма, а средство )
10. Владимир (Strange Device) 05.11.08 11:45
Считаю, что вынос каждого определения таблицы значений или массива в отдельную функцию или подпрограмму - не улучшает читаемость кода, а только затрудняет его чтение...
11. Lomok (lomok) 05.11.08 11:55
12. Альтаир (Altair777) 05.11.08 12:01
13. Алексей Константинов (alexk-is) 05.11.08 12:03
(10) (11) В данном случае не улучшает т.к. из названия функции совершенно не понятно, что она делает. Но могло бы улучшить, если функция вызывается несколько раз, если название отражает содержание, если добавить комментарии...
14. fez (fez) 05.11.08 13:50
Присоединюсь к (8)
Атцы рефакторинга (Кент Бек, Мартин Фаулер) настаивают на том, чтобы рефакторинг НАЧИНАЛСЯ с написания комплекта тестов. Для того, чтобы оный рефакторинг был БЕЗОПАСНЫМ.
15. Артур Аюханов (artbear) 05.11.08 16:09
1. Считаю, что работа по рефакторингу выполнена не полностью.
Например,
- что такое МДКласс и СоздатьМДКласс, совершенно непонятно :(
- ОбойтиХХХ также не очень раскрывает смысл кода :(
- переменная "Описание" также выполняет несколько функций и ее название не очень удачно.
.
2. По тестированию также не очень понятно, было ли оно выполнено или нет - а ведь структура кода довольно сильно изменилась.
.
3. Правильность работы кода, как исходного, так и полученного, не проверял, но терзают смутные сомнения - правильно ли использовать одну и ту же переменную "Описание" на разных уровнях кода? Ведь все время работаем с одним объектом :(
16. Артур Аюханов (artbear) 05.11.08 16:20
Также хорошо бы спец.блок
Код
Р = ложь;
Выполнить(Код);
Если Р = истина Тогда
Показать полностью

оформить в виде отдельного блока/метода/функции для обозначения того факта, что юзается спец.переменная P, которая должна быть обязательно использована в коде.
.
Например, ВыполнитьКод_ВозвращающийРезультатОтбораВСпецПеременную_P(Код)
.
А вообще подобное использование такой переменной со спец.названием - это неявная передача данных, что может быть чревато.
Вполне может быть, что проще изменить алгоритм - например, Код это просто выражение на языке 1С, которое должно вернуть Истину или Ложь.
А уж внутренний алгоритм выполнит
ИтоговыйКод = " P = " +Код;
и не будет никаких неявностей.
.
Тут не настаиваю, могут быть выбраны разные пути, в т.ч. и уже существующий :) - дело вкуса.
17. Артур Аюханов (artbear) 05.11.08 16:30
А.
Также не очень красиво
Функция ОбойтиМетаданные(П, Код)
функция и получает параметр П и возвращает ту же переменную П :(
А в описании задачи сказано, что П - изначально это пустая структура.
.
Фактически можно либо
1) Создать П внутри ОбойтиМетаданные и возвращать П как результат работы данной функции, а от параметра П избавиться.
2) или избавиться от функции, преобразовав ее в процедуру.
Для данной задачи ИМХО п.1 предпочтительнее.
.
Да и наименование параметра П также совсем не звучит :(
.
Б.
Функция ОбойтиАтрибуты(Атрибуты, ВидАтрибута, П, Описание, Код); - точка с запятой лишняя :( Данная опечатка, похоже, показывает, что код не тестировался :(
.
Данный блок определен как функция, а на самом деле ничего не возвращает - маленький косяк :)
18. - - (Rebelx) 05.11.08 16:31
минус
как минимум по тому, что считаю использовании попыток без обработки исключительных ситуаций грубейшей ошибкой
19. Артур Аюханов (artbear) 05.11.08 16:35
Также код внутри блока
Код
Для Каждого Класс ИЗ МДКласc Цикл
КонецЦикла;
Показать полностью

лучше выделить в отдельный метод типа СобратьДанныеПоМетаданному().
Код становится еще проще читать и исправлять.
20. - - (Rebelx) 05.11.08 16:38
также я не могу даже предположить, для каких задач могут пригодиться данные в таком виде
21. Альтаир (Altair777) 05.11.08 16:50
(20) > также я не могу даже предположить, для каких задач могут пригодиться данные в таком виде
прочитай внимательно шапку
В качестве подопытного кода была выбрана статья http://infostart.ru/blogs/688/. Не буду обсуждать полезность и нужность рассматриваемого кода, просто переставлю строчки кода так, что бы с ними можно было работать.
22. Олег Пономаренко (O-Planet) 05.11.08 21:51
Вообще имеет смысл что-то оптимизировать, не число буковок сокращая, а меняя саму суть. А придраться можно к чему угодно. Опять же, какова направленность оптимизации? Вариантов 4, как минимум: читабельность, функциональность, скорость, универсальность. Зачем в данном случае была предпринята оптимизация, и чем предложенный вариант лучше оригинала - не раскрыто. Лично мне, начинавшем программировать на С, в предложенном варианте сразу бросаются в глаза ляпы, которые никакой уважающий себя сишник не позволил бы себе. Например: зачем Описание=Новый Структура(); делать в цикле? Это ведь память. СоздатьМДКласс и СоздатьТЗ я бы сделал одним методом, суть у них одинакова, и сделал бы универсально:

Код
// Поля - перечисленные через запятую поля таблицы значений
Функция СоздатьТЗ(Знач Поля)
  ТЗ=Новый ТаблицаЗначений;
  Пока Поля<>"" Цикл
    П=Найти(Поля,",");
    Если П=0 Тогда
      Прервать;
    КонецЕсли;
    Поле=СокрЛП(Лев(Поля,П-1));    
    Поля=Сред(Поля,П+1);
    ТЗ.Добавить(Поле);
  КонецЦикла;
  ТЗ.Добавить(Поля);
  Возврат ТЗ;
КонецФункции
Показать полностью


ПС Poppy осознает свою гениальность... Она тоже стареет?
23. Артур Аюханов (artbear) 06.11.08 09:56
(22)
0. Смысл рефакторинга - в первую очередь, это повышение читаемости кода для возможности дальнейшего его исправления, сопровождения/изменения, повышения функциональности и т.д. и т.п.
.
1. Цитата:
>>СоздатьМДКласс и СоздатьТЗ я бы сделал одним методом,
>>суть у них одинакова, и сделал бы универсально:
Не согласен, как раз суть/смысл у них разные :) и нужны отдельные методы для точного понимания, что же делается этим кодом. А вот внутри этих спец.методов уже можно юзать как твое СоздатьТЗ, так и ручное заполнение.
.
2. По Описание = Новый Структура() - главное правило "Не нужно делать преждевременную оптимизацию" ! Нужно смотреть в профайлере уже после написания работоспособного кода и выполнения рефакторинга.
Mechanist; +1 Ответить
24. Олег Пономаренко (O-Planet) 06.11.08 16:16
>> Не согласен, как раз суть/смысл у них разные :) и нужны отдельные методы для точного понимания
Угу. Припоминаю эксамплы из Borland C 3.1 и BP 7 Там этот подход яростно практиковался, когда каждую мыслю в отдельный метод оформляли. Иногда достаточно обойтись комментарием, имхо, имея универсальный метод. Одно дело - десяток индивидуальных процедур, пусть даже с понятными названиями, другое - одна универсальная с комментариями в каждом конкретном месте ее вызова. Второе все-таки воспринимается проще. Этот подход практикуется, как понимаю, и у разрабов типовых конфигураций.

25. Артур Аюханов (artbear) 06.11.08 16:46
(24) Коммент для показа назначения кода очень часто не есть гуд :( при чтении вызовов подобных методов, как правило, неудобно изучать подобный код.
.
Гораздо проще и надежнее писать код так / называть методы так, что они не нуждаются в комментах, а их назначение понятно из их наименования.
.
Разработчики типовых - отдельная песня :)
В основном, конечно, очень грамотные решения, но и частенько совершенно отвратительный код :(
А уж любовь 1С к длиннющим блокам кода - это просто ужас :(
26. Олег Пономаренко (O-Planet) 06.11.08 22:18
(25) Думаю, лучше сочетать все-таки. 100 поименованных методов - тоже не есть гут. Когда метод унифицирован, то он лучше описывает алгоритм уже на уровне реализации. Даже тут: одно дело - две процедуры "СоздатьМДКласс()" и "СоздатьТЗ()"... Не посмотрев, я даже представить не могу, что может находиться внутри их, особенно в первой. Не вижу из контекста, что такое МДКласс... Если же использовать один метод и там и там то все становится ясно в контексте, плюс - я сразу понимаю, что есть на выходе "СоздатьМДКласс()". Т.е. если сделать

МДКласс= СоздатьТЗ("РегистрыСведений,Справочники,Документы, ... ");
ТЗ=СоздатьТЗ("Класс,Вид,ТЧ,Атрибут, ...");

- то все понятно и по структуре данных, и по их типу.
27. Артур Аюханов (artbear) 07.11.08 09:11
(26) 1. Я уже выше писал, что как раз название МДКласс и СоздатьМДКласс совсем неудачны и непонятны :)
2. В данном случае можно ипользовать и этот подход - все понятно и без комментариев.
Но с более сложным кодом такая фишка уже не пройдет :)
И самое главное - у правильного метода не нужно смотреть реализацию, все понятно из его наименования :) Например, твой же СоздатьТЗ не требует доп.комментов :)
28. Sensey Master (MSensey) 07.11.08 09:28
Минус
1. где описание проблем, которые были в исходном коде?
2. что должен был понять читатель этого блога?
29. Makdir (Makdir) 10.11.08 13:57
"1. где описание проблем, которые были в исходном коде?
2. что должен был понять читатель этого блога?"
Присоединяюсь. Важен не сам код решения, а содержание ошибок кода и методология. Лучше было дать сравнительную таблицу типа "верно", "неверно" и "почему именно так", а также рекомендации к составлению оптимального кода, при использовании которых начинающий программист сумел бы обойти "острые углы". Да и, в принципе, рефакторинг в моем понимании предполагает более глубокий анализ кода и структуры данных, а не просто исправление ошибок в исходном коде. Думаю многие со мной согласятся, что это слабое место 1С - инструментарий рефакторинга в ней оставляет желать лучшего.
30. Iom Nuerto (n949eo) 13.11.11 19:00
32. Алекс Ю (AlexO) 29.05.12 14:15
(5) BabySG,
Совершенно обоснованное мнение по "неопытному кодеру" - повторять н-раз один и тот же код - признак неопытности.

да неужели? вот откуда во всех типовых - десятки функций, делающих одно и тоже, но реализованные аюсолютно по-разному. Ну, и ошибки в них - разные...
Код идентичный повторять не нужно - делаешь вызов одной и той же фукнции.
А отмазки, типа: "Я не хотел дробить эту функцию на несколько, т.к. постоянно приходится ее тянуть в разные обработки." выглядят вообще смешно.

это почему еще? очень удобно не изобретать велосипед каждый раз (за что тут же, на инфострате, ратуют наверняка те же самые, кто пишет "смешно тянуть код в обработки" :) )
Это минус 1с-у - при такой дезорганизаванности и бессистемности операторов до сих пор нет официального репозитария стандартного кода и функций.
Зато прикольно будет, наверное, когда прицип обода поменяется и нужно будет изменять код в Н-местах вместо одного.

"прикольно" - это когда обработка номера документа в разных документах вызывает разные функции обработки.