Russian Qt Forum

Добрый день

Вот тут рефакторю помаленьку. Разумеется первый вопрос "А старый код работает? (чего лезть)". Да, работает, но любое изменение (а они требуются) обходится слишком дорого. Причем темп не ускоряется. Напр просидев дня 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 и вообще без методов, вместо них такие ф-ции как выше. Константность отсутствует. С именами правда хорошо. Показывать больше кода нет смысла - ничего нового не увидите.

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

Спасибо

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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

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

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

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

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

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

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

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

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

я не давал рекомендаций.


Достаточно большой проект (если он действительно большой) - это на самом деле кучка проектов по меньше.

Знать все от и до может быть не возможным в принципе.
Однако, обычно такой потребности и не возникает.

Обычно достаточно понимать и разбираться с теми вещами,
с которыми непосредственно приходится работать.

Вам нужно отдавать себе отчет в том, что именно вы хотите рефакторить.



но он не содержит конкретику.

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

Так, ну есть первые результаты. Все-таки сделал класс вместо "просто ф-ций". Плюс одна примитивная замена чтобы передавать меньше параметров. Пока все

- число методов класса 216. (Однострочных get/set нет, все пока public). Это оказалось для меня полной неожиданностью, я-то думал там штук 50, ну может 100, а оказывается айсберг намного больше.

- штук 6-7 ф-ций были написаны дважды (в местных namespace)  :)
- глобальный указатель на синглтон был объявлен (под разными именами) 3 раза
- пока перебирал текст, наметилось еще пара-тройка мест что можно удачно "схлопнуть" (мои расчеты оправдались)

Сколько новых багов появилось в рез-те моих действий - пока не знаю

Та действительно! Что именно рефакторить - не знает, не имеет ни цели ни плана, ну вот захотелось ему этим заняться! Все-таки это не так, да и вообще надо лучше думать о людях  :)

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

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

ох ты ж йожыг...

жаль, что всего кода не увидим, чтобы осознать глубину эээ... страданий, так сказать. Но кажется, что ад, батенька, сущий ад... Код то хоть на Qt или не особо? Просто кажется мне, что портирование на С++ дороже обойдется, чем заново наваять...

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

Я получил 3-4 новых бага с этими изменениями. Отлаживал в рантайм на 2 машинах, сравнивая значения переменных в старом/новом коде. Тоже не быстро, 2-3 дня ушло.

Не первый раз привожу эту ссылку (http://insidecpp.ru/antipatterns/lava_flow/), уж очень она мне нравится :) Решение "все переписать" - первое что приходит в голову, для этого не надо быть архитектором. Вот был "плохой" код, а мы напишем "хороший". Беда в том что нужно знать все детали ф-ционала, а это может потребовать неприемлемых затрат времени/денег. Часто все заканчивается на первом же вопросе заказчика, типа "ну хорошо, а когда новый код будет готов?" - и ответа нет, остается брать десятикратный запас

Сейчас перевожу 2 простых окна, одно просто просмотрщик картинки, другое оно же но плюс расчеты (рабочие, их можно не трогать). Здесь мне удалось выучить все "детали дизайна", ну и очень много старого кода рисования который с успехом может быть замещен на Qt. Но почему-то дело движется совсем не быстро, вот уже вторая неделька пробегает...      

В такой работе много рутины, аля написания кода. Смотрите, анализируйте, автоматизируйте :)

PS сам таким маялся, до сих пор штук 10-15 программ под каждую ситуацию в коде лежит.

Верес, займитесь лучше своими кнопками на цепях, не надо совать хобот туда где его оторвут  :)

А я в свою очередь желаю вам читать все темы и при желании отписываться :)

PS будьте взаимовежливыми и ещё скажите что я неправ :D

Ну а что, Главный Архитектор не прав был, что ли? :)

Я понимаю, что задача не простая, требует ресурсов и времени. У нас на фирме была такая же проблема - куча старого говнокода, который худо-бедно работал, иногда что-то там допиливалось, все плевались на него (ибо реально ГОВНИЩЕ было), но вот сесть и переписать - сразу же возникали вопросы "а кто это все оплатит? а сколько времени надо? а кто это делать будет?" ну и т.д. В итоге только на обсуждение того, надо переписывать или нет - ушло половина времени, за которое код таки БЫЛ ПЕРЕПИСАН (ну, программеры продавили в конце концов решение). И в итоге это решение окупилось.

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

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

Сомневался отвечать или нет, Ваш пост ничего нового не вносит. "По-хорошему надо все переписать" - то же самое сказал горе-архитектор :) Но как Вы представляете себе процесс переписывания? Похерить все старые исходники и хедеры и начинать все с чистого листа? Поверьте, это не катит. А иначе Вы немедленно попадаете в липкие лапы старого кода.

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

А что такое "автотесты"? Мельком глянул http://habrahabr.ru/post/238353/ (http://habrahabr.ru/post/238353/) - типичный хабровский треп

Любые тесты, автоматически тестирующие функционал. Масло масляное:) У нас 2 типа тестов - есть юнит тесты на гугл-тестах/qt-тестах, к-ые тестируют конкретные классы. И тесты, проверяющие работу сервиса по методу черного ящика - что-то на вход дали (сообщение), что-то ожидаем увидеть (другое сообщение). Понятное дело, для гуев второе не подходит, но подходит разделение гуй\негуй и юниттесты негуя.

Понял, спасибо. Расчетная часть того же возраста, но гораздо более благополучная, плюс я ее знаю лучше, многие места сам делал. Тестов там негусто, но есть. А вот c UI - полный ппц   :'(