LINUX.ORG.RU

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

 ,


3

8

Всем привет!

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

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

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

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

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

★★★★

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

Ответ на: комментарий от LongLiveUbuntu

У нас столько C++ Senior developer платят.

вот тут за yaml с json платят по 400к: Ищу DevOps/Release Engineer (300-400к net, офис/гибрид/удаленка)

если ты ищешь денег, то можно попробовать поискать больше на других вакансиях…

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

Удаленная работа сейчас очень много где. Я сам например живу в бедном регионе, но мне тут нравится, поэтому я никуда не уезжаю.

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

В принципе, да, но это порой просто не заходит. Так же как тебе не заходит вариант переехать в богатый регион. Сам живу в провинции, хотя в Москве той же за мою работу платят в 3-5 раз больше. Но вот просто не хочу дёргаться, зона комфорта, всё устраивает 🤷

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

Если конечно, ты не собираешься называть сравнение с паттернами анализом.

Например, cppcheck именно так и работает, но гордо называется статическим анализатором :)

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

Работает/не работает это объективный критерий, а через задницу или нет - субъективный.

Код может быть «не красивым», нарушать какие-то правила «хорошего тона», но при этом не быть хрупким и достаточно очевидным, чтобы не создавать проблем в поддержке.

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

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

annulen ★★★★★
()

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

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

Какой язык? Есть статические анализаторы? Линтеры?

Rust, clippy, cargo-audit, функциональные тесты на питоне в CI, модульные тесты, док-тесты включая запрет на недокументированные публичные объекты (не шучу), комменты на русском везде, чтобы новые разрабы могли легко вникнуть при низком уровне.

Собственно, прям в main.rs

//! Нельзя менять, данный блок позволяет избавиться от некачественного сопровождения кода
#![deny(
    missing_docs,
    missing_debug_implementations,
    trivial_casts,
    trivial_numeric_casts,
    unsafe_code,
    unstable_features,
    unused_import_braces,
    unused_qualifications
)]

При любых варнингах\ошибках CI падает и не дает влить мерж в главную ветку — такая политика. Но кажется что статические анализаторы и линтеры в других языках могут сильнее автоматизиовать выявление говнокода.

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

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

Спасибо, кажется в эту сторону и надо идти

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

[я хз почему ТСа сразу записали в самодуры, а нижеследующее просто вспомнилось] Несколько лет назад один малолетний мразёныш-тимлид целый месяц трахал мне мозги, докапываясь до имён переменных и функций. При этом срать он хотел на имена, ему было лишь бы власть показать.

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

DDD

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

Абсолютно не так, надо просто объекты, которые отвечают за что-то одно складывать в одном месте и это место называется доменом.

Главный объект в домене - это доменный сервис, у него могут быть любые методы.

Чтение\запись в домене через интерфейс, который называется репозиторием.

Бизнес логика - это функции, которые работают с интерфейсами и не связаны на прямую со всякими структурами и объектами.

Ну и правила видимости, инверсии зависимостей - там тоже все просто.

Почитай про это, лишним не будет

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

На месте начальства я бы уволил ТСа, потому что он вместо того, чтобы работать, тратит целые рабочие дни на какое-то др*чево

А как же шаринг компетенций в команде?

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

За проект отвечаешь ты?

Да

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

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

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

слушай, ну может и не так уж и не прав был тот тимлид.

глядя на имена твоих переменных:

азиатские_шлюхи_получают_по_заслугам большие_чёрные_стволы ГлубокаяДыркаТвоейПодруги

и прочее-прочее.

я б тя тоже забанил. а он всего лишь мозги траха. да еще и месяц, всего. добрый был.

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

При этом срать он хотел на имена, ему было лишь бы власть показать.

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

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

Он докапывался к коротким именам внутри коротких методов (типа вместо i внутри цикла надо писать elementIndex), и к фразировке длинных имён API-методов, где был только camelCase.

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

А, так это Москва. А я в России, г. Краснодар живу.

Удалёнка существует.

Да, ты будешь работать на компанию из Москвы(скорее всего), но жить в г. Краснодар.

видимо компании Краснодара не хотят платить по рынку, поэтому вероятно будут терять людей.

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

вместо i внутри цикла надо писать elementIndex

Это, наверное, перебор. i, j, k — это ведь торадиционные, всем понятные названия для таких переменных. Ёжику очевидно, что это целочисленный индекс.

Вот аргумент функции в каком-нибудь map/forEach/filter, конечно, стоит обозвать подлиннее, потому что он может быть чем угодно. response.data.map(user => user.isMarried) выглядит лучше, чем response.data.map(x => x.isMarried) — сразу видно, что речь идёт о списке пользователей, например.

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

вроде как неудобно сказать чтоб переделал без обоснования почему.

Вам очевидно насколько это является серьезной проблемой, причём конкретно вашей проблемой как тимлида / ментора (не знаю уж на какую роль вы претендуете)? Если вы не можете объяснить своему начальству на что тратятся деньги конторы (в форме времени сотрудников) - вы или что-то совсем неправильно делаете, или не на своём месте. Мои 2 копейки.

bugfixer ★★★★★
()

Кажется автор просто забыл что значит быть новым разработчиком на проекте

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

Автору нужно инвестировать больше времени на постановку задачи. Буквально писать вот так: у нас в файле ааа есть класс ббб, в него нужно добавить метод ввв. Тут же пояснять бизнес логику. Тут же пояснить выбор места для нового метода: это уровень хранения данных, а вот это уровень бизнес логики

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

anonymous7
()

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

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

Вот жил-был, например, обычный человек, а как приехал в Москву — так сразу москвичом стал.

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

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

Это, наверное, перебор. i, j, k — это ведь торадиционные, всем понятные названия для таких переменных. Ёжику очевидно, что это целочисленный индекс.

А потом получается: «The maximum number of nested for loops is 18 because that’s the number of letters between i and z.»

Оригинал статьи 😊

aol ★★★★★
()

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

Какие нафиг созвоны на код-ревью? Вы там блин делаете систему управления для самолётов что-ли?

upcFrost ★★★★★
()