Наименования

KateBushueva commented 3 years ago

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

KateBushueva commented 3 years ago

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

Sergey111991KV commented 3 years ago

Если именуюшь Т, то только тогда, когда это общая договоренность)

pf03 commented 3 years ago

Вообще таких наименований в боте у меня мало и они используются только для ключевых позиций T - Transformer, E - Error, S - State, MT - Monad Transformer Class (включает в себя остальные классы). Ну если это не очевидно, изменю.

KateBushueva commented 3 years ago

вообще не очевидно, T может быть Transformer, а может быть Table (например, для БД), E может быть Error, а может быть Environment. Не должно быть необщепринятых сокращений

pf03 commented 3 years ago

вообще не очевидно, T может быть Transformer, а может быть Table (например, для БД), E может быть Error, а может быть Environment. Не должно быть необщепринятых сокращений

Надеюсь, это не относится к локальным сокращениям? То есть я могу сократить object до o например в parseUpdate o = do ... Насколько я читал в статьях, чем "более локальная" сущность, тем короче название

Sergey111991KV commented 3 years ago

Это холивар я думаю) если всем понятно и приемлемо, то конечно можно. Бот у тебя хороший, ты видно что старался выбирать названия минимальные, просто... это довольно таки не очевидно бывает, что имеется ввиду. А в parseUpdate я бы подумал, что за о? или что за object?) почему например не update, раз функция обрабатывает его? а вдруг у нее несколько аргументов и еще один object будет? тогда object1 и object2?) я на проекте увидел название из восьми слов в одном - и его спокойно приняли, чтобы не было двоякости в понимании. Пускай ты будешь его дольше читать, но поймешь точно что это)

pf03 commented 3 years ago

Переименовал E - Error, S -State, T - Transformer, MT - MTrans. По каким наименованиям еще вопросы?

KateBushueva commented 3 years ago

к примеру, модуль Transformer.Functions функция run - название ок, внутри функции: ec, dlc, s, ea и пр. - должны быть понятные имена переменных

pf03 commented 3 years ago

к примеру, модуль Transformer.Functions функция run - название ок, внутри функции: ec, dlc, s, ea и пр. - должны быть понятные имена переменных

Да, такие наименования у меня встречаются по всему коду, и я могу объяснить логику. Как правило, это сокращение от типа или от функции: ec - Either Error Config, dlc - defaultLogConfig, ea - Either Error a, s - State Я использую HLS подсветку синтаксиса, поэтому при наведении курсора сразу понятно какой тип имеет переменная и откуда такое название. Для меня это удобно. К тому же это имена локальные и во внешнем коде не используются. Можно придумать названия подлиннее, что приведет к раздутию кода даже для простых тривиальных функций. Либо другой вариант, отказаться от одноразовых переменных, то есть вместо

ea <- runExceptT $ runStateT (getTransformer m) s0
        case ea of ...

писать сразу

case runExceptT $ runStateT (getTransformer m) s0 of ...

Не могу найти статью, но читал что чем "более локальная" сущность, тем короче название

KateBushueva commented 3 years ago

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

KateBushueva commented 3 years ago

имя uidFromFile меня каждый раз сбивает https://github.com/pf03/newBot/blob/7ba8545adbe74e4705cc72f91994aedeeb848d17/src/Logic/Bot.hs#L21 пусть будет uidFromFileEnabled

pf03 commented 3 years ago

добавил более длинные и понятные имена для локальных сущностей

KateBushueva commented 3 years ago

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

переменные/классы обычно существительным или словосочетанием
булевые переменный обычно с приставкой is... / has... / ...Enable или что-то подобное, то есть как вопрос, на который будет ответ да/нет
функции обычно содержат внутри глагол, что эта функция делает (то есть не application, а runApplication).

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

pf03 commented 3 years ago

Исправил и прошелся code spell checker, как советовал Сергей

catdarick commented 3 years ago

https://github.com/pf03/newBot/blob/5c8e387215807a19862848cc28f94b6752ddf59d/src/Logic/App.hs#L33 Не очень удачное название. Функция обрабатывает апдейты: создаёт и отправляет ответы на них, думаю, handleUpdates подойдёт лучше

pf03 commented 3 years ago

исправил

pf03 / newBot

Наименования #7