LINUX.ORG.RU

Ревью кода или психология мидла

 ,


3

8

Всем привет!

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

И любит он делать херовый код (плохой нейминг, непонятные и ненужные абстракции, каша в логике). Если пнуть, то обычно исправляет. Но я уже заманался его пинать, одни и те же ошибки в каждом МР. Уволить?! Как говорит начальство — не можем, бюджет не позволяет платить больше кому-то, а найти нового человека сейчас очень сложно.

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

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

Что обычно делают в таких ситуациях? Писать что все фигня, давай по новой и закрывать мерж? Или есть какие-то техники? Подскажите или тыкните что почитать?

★★★★

Последнее исправление: maxcom (всего исправлений: 1)

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

Jameson ★★★★★
()

Для начала нужно определить, проблема в нём или в тебе.

Понятие «красиво» не формализуется, если проблемы ловятся каким-либо стат анализатором - подключать его.

Требуй юнит тесты, обычно если код плохой, то тестить его больно.

Зафиксируй документ с требованиями.

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

ya-betmen ★★★★★
()

одни и те же ошибки в каждом МР. Уволить?! Как говорит начальство

Начни с себя. Не каждый будет делать код, как ты хочешь.

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

Он не макака, твои хотелки делать, а творческая личность. Уважай мнение других

есть мидл в условном подчинение т.е. формально мы на одном уровне

Гм на так не в твоем подчинении. Почему у него нет таких же прав, как у тебя?

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

Правильно делает, ты же первый начал затягивать ревью.

ivanich10
()
Последнее исправление: ivanich10 (всего исправлений: 3)
Ответ на: комментарий от HE_KOT

значит, проблемы нет ;D

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

goingUp ★★★★★
()
Ответ на: удаленный комментарий

Дык проект общий, а я за него ответственный. Не хочется мержить в код что-то плохое. Или я не так понял как это сделать

Его код ничем не хуже, чем твой. Не нужно мнить себя выше других. Он просто ДРУГОЙ.

ivanich10
()
Ответ на: комментарий от ivanich10

Не каждый будет делать код, как ты хочешь.

А вот этого вообще никогда не происходит. Принятие этого у меня лично заняло время далеко отличное от нулевого.

bugfixer ★★★★★
()
Ответ на: комментарий от ya-betmen

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

У нас DDD и чистый код, абстракции больше определяются через DDD, а чистота кода соответственно правилами CleanCode. Но кто бы их читал кроме меня?!

AntonyRF ★★★★
() автор топика

Мимоходом поинтересуйся, не с бейсика ли он начинал.

Хи-хи, старшее поколение сообщает, что при Союзе таких отправляли в подшефный колхоз, чтобы глаза не мозолил.

Irma ★★
()
Последнее исправление: Irma (всего исправлений: 1)
Ответ на: комментарий от AntonyRF

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

Не не не, так не надо. У вас же там какие-нибудь operations имеются? Вот пусть они перца и вызванивают. Я думаю - пары раз хватит.

bugfixer ★★★★★
()
Ответ на: комментарий от ivanich10

Начни с себя. Не каждый будет делать код, как ты хочешь.

А потом codestyle, DDD, и все наши требования идут лесом. Уходишь на больничный, через две недели там ахтунг

Он не макака, твои хотелки делать, а творческая личность. Уважай мнение других

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

Гм на так не в твоем подчинении. Почему у него нет таких же прав, как у тебя?

Потому что меньше опыта, а отдел со строгой иерархией из двух человек не формируют

Правильно делает, ты же первый начал затягивать ревью.

Так может правильно было бы почитать книг? А не думать о том какой плохой дядя затягивает мое творческое ревью? =)

AntonyRF ★★★★
() автор топика
Ответ на: комментарий от sparkie

Низзя! Проект-то ТСа, отвечать ему.

Кто девушку поит, тот её и танцует. Если «в ответе» - то можно (и нужно) заставлять делать «как хочется». Главное не переусердствовать. Работает - и ладно. Это главный критерий.

bugfixer ★★★★★
()
Ответ на: комментарий от goingUp

Ну проблемы могут возникнуть, когда с этим наркоманским кодом будет потом работать кто-то другой)

Проблемы есть в старом проекте, который писали абы как, в итоге на третий год жизни таски выполняются с 1% скоростью от изначальной из-за большой связанности и запутанности кода. С помощью DDD решили растаскать проект на домены сделав модульный монолит в котором стандартные компоненты типа - use-case’ы, доменные сервисы, entities, aggregates, repositories. Все делиться на несколько слоев, слой центральный с сущностями данных, слой application с бизнес логикой и слой c инфраструктурой. БЛ использует интерфейсы чтобы не зависеть от остального кода на прямую. ИЧСХ это в целом все что мы делаем в проекте и по каждому пункту у нас что-то да выскакивает на ревью. То бизнес логика в репозиторий улетает, то у нас агрегат в ValueObject, то еще какие-то сюрпризы. Да может это сложно сразу залезть и писать по паттернам, но уже прошел ГОД и мы на том же месте!

AntonyRF ★★★★
() автор топика
Ответ на: комментарий от AntonyRF

Так может правильно было бы почитать книг?

Ну вот кстати можно сделать еще одно конструктивное предложение, если ты думаешь, что проблемы от того, что он не разбирается в DDD, можно поговорить с руководством чтобы ему выделили время на прочтение этих самых книг, или хотя бы getting started статей, потому что ожидать, что он будет их читать в свое личное время на выходных, по-моему напрасно)

goingUp ★★★★★
()
Ответ на: комментарий от goingUp

Ну вот кстати можно сделать еще одно конструктивное предложение, если ты думаешь, что проблемы от того, что он не разбирается в DDD, можно поговорить с руководством чтобы ему выделили время на прочтение этих самых книг, или хотя бы getting started статей, потому что ожидать, что он будет их читать в свое личное время на выходных, по-моему напрасно)

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

AntonyRF ★★★★
() автор топика
Ответ на: комментарий от bugfixer

ЧСХ, с точки зрения менеджмента «обсираться» в подобных случаях скорее будет автор, чем этот проблемный тип. Потому по отчётам – он быстро закрыл таску, потом пофиксил 100500 тикетов, а потом разгребать это говно наняли ещё 10 человек и он стал начальником. А заранее всё продумал и написал по уму – закрыл всего 1 таску за большее время.

Поэтому такая ситуация встречается повсеместно и плодится в IT как грибы после дождя.

snizovtsev ★★★★★
()
Последнее исправление: snizovtsev (всего исправлений: 1)
Ответ на: комментарий от frunobulax

Вычитать из зп за косяки?

Не, зп и так не большая (150к), а так же это добавит стресса и он может еще хуже начать работать, надо как-то мотивировать расти, а не убивать желание работать

AntonyRF ★★★★
() автор топика

3-4 часа ревью - это очень много. Сколько же он писал код - неделю? Декомпозируйте задачи сильнее, не должно быть столько на ревью. Плюс к этому - изменять код на ревью очень дорого. Надо оговаривать что должно быть в коде на планировании, раз чел сам не вывозит.

stave ★★★★★
()
Ответ на: удаленный комментарий

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

stave ★★★★★
()
Ответ на: комментарий от AntonyRF

Так формализуй. Подпиши у кого надо и оставляй ссылки на номер правила в коментах, как на ЛОРе.

Ну и да, чтотс тестированием то?

ya-betmen ★★★★★
()
Последнее исправление: ya-betmen (всего исправлений: 1)
Ответ на: комментарий от stave

Ещё как вариант перейти к тдд - перед тем, как приступать к логике пусть напишет тест кейсы и быстренько сделайте их ревью. Дальше формально все просто - прошел или нет.

stave ★★★★★
()
Ответ на: комментарий от AntonyRF

Оформить свои требования к стилю в виде документа

Вот это хорошее предложение.

пытается затягивать тимлидов на обсуждение ревью и затащить говнокод таким образом

Это тебе на руку, когда есть документ. Явно указываешь на несоотвествие кода документу, чтобы и тимлиды это видели. После того как тимлиды на это посмотрят один раз, два, три, их уже не надо будет убеждать в проф(не)пригодности этого человека.

rupert ★★★★★
()
Ответ на: комментарий от AntonyRF

ты спалился Как писать резюме? Если ты знаешь все, но поверхностно.

С таким резюме ты макс. джун. Если 10 лет кодишь «поверхностно», да еще на одном месте - то твой код - кусок сам знаешь чего. А уже незнание асинхронщины сегодня - это вообще моветон.

ivanich10
()
Последнее исправление: ivanich10 (всего исправлений: 2)

Кот в студию, будем оценивать. Можно и плохой, и исправленный/образцовый варианты — никто не уйдёт обиженным.

Nervous ★★★★★
()

если после изначального поста я сомневался, то после такого коммента

У нас DDD и чистый код, абстракции больше определяются через DDD, а чистота кода соответственно правилами CleanCode

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

Lrrr ★★★★★
()

Про тдд и дробление уже написали, можно ещё обложить код жесткими линтерами, да с кастомными правилами, которые сразу будут бить по рукам, остальной уже скорее логический стиль формализировать, хотя бы небольшим А4: exceptions не используем и тд, и тп.

ac130kz ★★
()
Ответ на: комментарий от Lrrr

По-любому. Еще что характерно — все это говно по большей части в мире джавы и шарпа живет, остальные живут без всего этого и горя не знают.

anonymous
()

Бегло глянул тред. Правильно я понимаю, что ivanich10 это тот самый мидл, который узнал себя? Ну и ну, бывают же совпадения 😊.

Virtuos86 ★★★★★
()

Какой язык? Есть статические анализаторы? Линтеры? Конечно внедрение такого требует времени, но оно того стоит, если разумеется, соблюдать основное правило – не заливать новые ворнинги и подключить к CI чтоб падало при добавлении нового ворнинга. И постоянно форсить добавления новых правил все более и более ограничивающих свободы написания кода, потому как способов написать дичь люди знают много, но где-то 90% обычно покрывается статическим анализаторами.

Я сам работал в команде где был код ревью, так вот правила там плавающие были, в зависимости от настроения людей, когда делать нечего а на трекере задачи какие-то нужно отметить то все кидались ревьювить код, это был ад чтоб что-то закомитить :) А когда работы много можно было дичь закомитить и никто даже не колыхнется при реврью – одобрено.

Aber ★★★★★
()
Последнее исправление: Aber (всего исправлений: 1)
  1. Определить зоны ответственности. Те должно быть четко оговорено и зафиксировано, кто отвечает за проект.

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

  3. Провести установочное собрание и озвучить правила игры. Кто не принял правила - убирать с проекта.

PS Правила игры могут быть даже уровня: Я начальник, ты дурак. Главное, чтобы они были явно озвучены и приняты командой проекта.

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

Norgat ★★★★★
()
Ответ на: комментарий от AntonyRF

DDD

Это когда все идет во главу перформанса? Типа если у нас есть 10 классов объектов которые могут храниться в одной коллекции, то вместо одной коллекции и свитча мы пишем 10 коллекций под каждый тип чтоб избавиться от ветвлений которые саботируют линейную обработку данных?

Aber ★★★★★
()
Ответ на: комментарий от Aber

Это когда все идет во главу перформанса?

Не, то Data-Oriented Design, выжимаем всю производительность до такта (в ущерб всему остальному). DDD это Domain-Driven Design, пляшем от предметной области.

Понапридумывали аббревиатур, понемаешь.

Nervous ★★★★★
()
Последнее исправление: Nervous (всего исправлений: 1)