Refactoring

[Igors]

Добрый день

Вот тут рефакторю помаленьку. Разумеется первый вопрос "А старый код работает? (чего лезть)". Да, работает, но любое изменение (а они требуются) обходится слишком дорого. Причем темп не ускоряется. Напр просидев дня 2 и наконец чего-то добившись (хотя неизвестно в плюс или минус), в след раз дело быстрее не идет. Масса времени уходит чтобы понять "а что делает это", "а то", пока чего-то разобрался - день кончился. Код написан в суровом процедурном стиле. Пример

Код

C++ (Qt)
bool TrackSingleTime (Layout* theLayout, Window &window, long theService, short lockOption,
		  short thePanelNumber, short theLineNumber, short theFieldNumber,
		  Object* theElement, long theDataPart, short theKeyNumber,
		  Point thePoint, double * theKeyTime );
 

Это конечно не самый короткий спысок аргументов, но и далеко не самый длинный. Все классы struct и вообще без методов, вместо них такие ф-ции как выше. Константность отсутствует. С именами правда хорошо. Показывать больше кода нет смысла - ничего нового не увидите.

Как бы Вы поступили? Какие есть предложения? Отказаться от этой работы не могу, должен сделать.

Спасибо

[gil9red]

Смотря на параметры функции хочется сгруппировать их в структуру

[m_ax]

Написать свою обёртку над всем этим низкоуровневым сишным слоем. И здесь, конечно, нужно вначале продумать архитектуру: какие классы выделить, их взаимодействие, интерфейс и т.д.. Но это, конечно, не один день.. Но.. 

[Igors]

Смотря на параметры функции хочется сгруппировать их в структуру

Мне тоже  Вот именно сейчас сделал структуру из 4 интов, и в вызовах заменяю

Код

C++ (Qt)
short thePanelNumber, short theLineNumber, short theFieldNumber,
// на 
theLoc

Таких вызовов неск десятков - все ж будет легче. Но и такая примитивная замена - часов 8 редактирования. Остальное объединить - ну не видно оснований.

Написать свою обёртку над всем этим низкоуровневым сишным слоем. И здесь, конечно, нужно вначале продумать архитектуру: какие классы выделить, их взаимодействие, интерфейс и т.д.. Но это, конечно, не один день.. Но.. 

Т.е. не пытаться (во всяком случае особо) улучшить, а как-то сверху управлять, так что ли?. Хмм.. было бы конечно хорошо, но пока не вижу каким образом.

Собственно посвящено это все одному громадному окну в котором раскладушки, спрыдшыт и чего там только не напихано (даже редактор audio). Много (пожалуй больше половины ф-ционала) = операции мыши, разнообразные драги и.т.п.  Сделано прямолинейно

HandleMouseDown - в какое место тыкнули? (их там десятка 3)
   - HandleMouseDownInTimeLine - ага, попали в timeline, там тоже по-всякому
   ....
    TrackSingleTime  - та ф-ция что я привел

[Bepec]

Ну в принципе замена на структуры это вариант.

При желании и однотипности задачи, можно написать спокойно парсер, который будет заменять параметры структурой и менять их по коду.
Потратить час, мб 2, зато потом решать эту задачу минут за 5-10. 

PS хотя думаю данный функционал в каких нить утилитах для рефакторинга есть уже давно. К сожалению не интересовался.

[Old]

Ну в принципе замена на структуры это вариант.

Скажите, а чем замена нескольких параметров функции одной структурой упрощает дальнейшее сопровождение кода?

[Bepec]

Позволяет сгруппировать IN параметры, тем самым упрощая понимание кода.

Вместо "дай тарелку, дай кашу, дай ложку, дай мяса" получаем "дай пожрать"

[Авварон]

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

[Old]

Позволяет сгруппировать IN параметры, тем самым упрощая понимание кода.

Вместо "дай тарелку, дай кашу, дай ложку, дай мяса" получаем "дай пожрать"

А если параметры логически не группируются? Добавить для каждой такой функции свою структуру?
По моему, это просто трата времени.

Перед началом рефакторинга, нужно хорошо понимать что мы имеем сейчас и что мы хотим получить. Например, у нас есть куча функций, которые лезут во все подсистемы и мы хотим заменить их несколькими своими классами, которые хорошо впишутся в общую архитектуру и скроют в себе кучу этих функций с их "подробностями". Или у нас есть сишный код, а мы хотим из него получить ОО, т.е. набор классов, объекты которых впишутся в нашу общую систему.

Нам показали прототип какой-то функции и не написали главного, а что же хотелось бы получить в конце работы.

[Igors]

Начало верное.

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

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

Часто (как в приведенном примере) обилие аргументов вызывается "модальностью" ф-ции. TrackXXX получает управление когда мышь нажата и какой-то драг возможен. И возвращает управление когда драг полностью закончен и мышь отпущена. Отсюда и аргументы Window, Point и др. Довольно долго размышлял не переделать ли это поведение на безмодальное (mouseDown сразу вернет управление, а затем уже в moiseMoved). Пришел к выводу что не стоит, старое решение выгоднее.

[Авварон]

Т.е. Вы точно знаете что верно а что нет.

Я всю сознательную жизнь переписываю чужой говнокод:(

Часто (как в приведенном примере) обилие аргументов вызывается "модальностью" ф-ции. TrackXXX получает управление когда мышь нажата и какой-то драг возможен. И возвращает управление когда драг полностью закончен и мышь отпущена. Отсюда и аргументы Window, Point и др. Довольно долго размышлял не переделать ли это поведение на безмодальное (mouseDown сразу вернет управление, а затем уже в moiseMoved). Пришел к выводу что не стоит, старое решение выгоднее.

Я не очень понимаю. Ф-ия крутит эвент луп внутри себя? Это, зачастую, плохая идея - можно случайно свалиться в рекурсию эвентупов. Исключение - модальные диалоги.

Вообще, есть правило, в книжке про написание хорошего кода - ф-ия должна делать ОДНУ вещь. Типа если у вас есть ф-ия, в к-ой, скажем, есть иф, то ветки ифа надо вынести в 2 ф-ии - тогда верхняя ф-ия будет делать 1 вещь - переключать поток управления. Или вы там проверяете параметры, а потом вызываете ф-ию, к-ая делает работу (в предположении что параметры валидны). Второе, кстати, очень часто полезно при рекурсивных вызовах. Понятно, что это крайность и так делать почти никогда не надо, но надо понимать - вот ф-ия делает то-то, то-то и то-то. Если она делает слишком много (больше двух-трех вещей) - ее надо разбивать.

Еще полезно группировать связанные параметры (не путать с группировкой параметров, имеющих один смысл - например, координаты - они симметричны, но x от y не зависит). А вот widget и его top-level widget (окно) - вполне (можно получить топ-левел по указателю на виджет). Не связаны ли у вас Window и Layout, к примеру? Соответственно, иногда можно передать 1 параметр вместо нескольких (и получить их внутри ф-ии), а можно сгруппировать такие параметры в структуру (ака контекст). Например, у меня есть двухуровневое дерево и мне надо работать с моделиндексами. Тогда контекстом будет текущий индекс (мы не знаем, какого он уровня), индекс нижнего уровня (равен текущему, если текущий на нижнем, нулл иначе) и индекс верхнего уровня (парент текущего или сам текущий соответственно). Вуаля, у нас минус 3 параметра, локализовано их получение (ф-ия currentContext) - мы не опечатаемся, взяв не того сиблинга и нам надо проверять только ту часть контекста, с к-ой мы работаем. Да, в некоторых ф-иях нам вообще не нужен нижний уровень, но за удобство приходится платить.

[Igors]

Я не очень понимаю. Ф-ия крутит эвент луп внутри себя? Это, зачастую, плохая идея - можно случайно свалиться в рекурсию эвентупов. Исключение - модальные диалоги.

Вообще, есть правило, в книжке про написание хорошего кода - ф-ия должна делать ОДНУ вещь.

Да, вторичный eventLoop. Про "одну вещь" конечно все читали, но в жизни получается далеко не так однозначно. Напр мы можем рассматривать track как ОДНУ операцию, для этого есть веские основания, а иногда мы даже обязаны так делать. Если же размазывать его по mouseDown, mouseMove, mouseRelease, то надо делать что-то типа CTrackContext. И в данном случае я такого не вижу - нет никакой заметной общности между разнообразными траками (да она бы сразу и почувствовалась).

Не связаны ли у вас Window и Layout, к примеру?

Да конечно связаны, даже есть ф-ция извлекающая Window. Ну и конечно все ф-ции надо сделать методами Layout. Но каждое изменение - это дни. Кручусь чтобы подкормить заказчика реальными улучшениями и выкраиваю время на переделки.

[_Bers]

Масса времени уходит чтобы понять "а что делает это", "а то", пока чего-то разобрался - день кончился.

Утрируя, есть два способа:

1. По прототипу.
Это означает, что старый код воспринимается как некий рабочий прототип.

Смысл в том, что бы написать с нуля принципиально новый механизм, по прототипу существующего.
После чего старый прототип можно будет отправить в мусорное ведро.
(это намного быстрее и проще, чем писать полностью с нуля.
перед глазами как минимум есть рабочая версия,
которая иллюстрирует "как это можно сделать",
и понимание самой идеи: чего хотим получить в итоге)

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

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

2. Рефактор по живому.
Здесь нужно быть хирургом, который понимает анатомию проекта.
Важно осознавать, какие будут последствия правок.

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

И так, за несколько итераций, код постепенно "выпрямляется".

3.
Лично я выделяю функционал, который в существующих условиях реально покрыть тестами.
(Это означает, что он более менее сохраняет модульность. Его можно хоть как то изолировать)

Сам рефактор осуществляется под защитой тестов.

Затем опять перехожу к пункту 3.

Как бы Вы поступили? Какие есть предложения? Отказаться от этой работы не могу, должен сделать.

У вас проблема: понять "что делает это".

Другими словами, у вас нет понимания даже дизайна использования.
Не говоря уже об анатомии проекта.

Без понимания дизайна, рефакторить код нельзя.

Рефакторить можно только когда есть понимание:
1. Что можно улучшить.
2. Как это можно улучшить.

[Igors]

У вас проблема: понять "что делает это".

Другими словами, у вас нет понимания даже дизайна использования.
Не говоря уже об анатомии проекта.

Без понимания дизайна, рефакторить код нельзя.

Рефакторить можно только когда есть понимание:
1. Что можно улучшить.
2. Как это можно улучшить.

Ага, "нет даже", типа "вот идите и изучайте дизайн!" Кстати Вы далеко не первый дающий подобные рекомендации. Неск раз проверялся такой вариант

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

Ответы звучали по-разному, но суть всегда одна

$50K (минимум названный самым скромным) и срок исполнения от 1 года (меньше не было ни разу)

Подробности

- Гмм... ну а чего ж так дорого и долго?
- потому что задача объективно сложна

- А где гарантии что мы получим гибкий код который сможем успешно развивать?
- моя высокая квалификация (знание буста, std, многих платформ и.т.д.  и.т.п.)

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

Возвращаясь к теме. Что хочется/нужно сказано в первом посте

Да, работает, но любое изменение (а они требуются) обходится слишком дорого.

Нужно чтобы код стал более гибким и его можно было легче развивать. То что это "не один день" (и не два) и так всем ясно - мы никуда не торопимся. Может нужен пример как об этот код постоянно "разбиваются коленки"? Не вопрос, таких случаев хоть отбавляй

[Bepec]

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

Не 50к $, не 1 год, а 5 раз по 10к $ и ещё пару десятков на разработку вменяемого стандарта кода для вашей организации

Без стандарта вы будете шить лоскутное одеяло и стоимость изменений ещё и повыситься. Со стандартом вы постепенно, модуль за модулем приведёте всё к единому виду и одинаковой гибкости

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