pf03 / newBot

BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Замечания по коду pt. 2 #19

Open catdarick opened 3 years ago

catdarick commented 3 years ago

Лучше сделать так, чтобы было сразу видно, что это транформер над IO, а также конкретизировать название, Transformer - слишком абстрактно :) Например type BotStateT = StateT BotState или type BotStateIO = StateT BotState IO

https://github.com/pf03/newBot/blob/ac003281dad75b23a4fa7a26925fb81557d0910c/src/Transformer/Types.hs#L15

catdarick commented 3 years ago

В этом модуле, мне кажется, перебор с тайп алиасами. Они заставляют искать определение, чтобы узнать что за тип на самом деле используется, это не всегда удобно. Проблему они тут никакую, вроде, не решают. И ещё для Bool полей стоит подбирать такие названия, чтобы условия более легко и грамотно читались (*if enabled* == *если включено* против *if enable* == *если включить*) https://github.com/pf03/newBot/blob/ac003281dad75b23a4fa7a26925fb81557d0910c/src/Interface/Log/Types.hs#L34 https://github.com/pf03/newBot/blob/ac003281dad75b23a4fa7a26925fb81557d0910c/src/Interface/Log/Types.hs#L36 https://github.com/pf03/newBot/blob/ac003281dad75b23a4fa7a26925fb81557d0910c/src/Interface/Log/Types.hs#L38

Этому полю лучше дать более конкретное название, не очень понятно для чего оно нужно https://github.com/pf03/newBot/blob/ac003281dad75b23a4fa7a26925fb81557d0910c/src/Interface/Log/Types.hs#L42

catdarick commented 3 years ago

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

https://github.com/pf03/newBot/blob/ac003281dad75b23a4fa7a26925fb81557d0910c/src/Interface/Log/Functions.hs#L27-L29

С текущим подходом можно легко ошибиться, просто забыв вызвать её в начале другой функции. Для этой задачи лучше использовать функцию высшего порядка, обычно, при подобном назначении, их именуют с префиксом with. Эта функция должна принимать те же параметры, что и setSettings, а также действия, которые необходимо в неё "звернуть". То есть она будет иметь тип withCustomSettings :: ColorScheme -> Enable -> FuncName -> Transformer a -> Transformer a. С учётом того, что эти настройки хранятся в стейте, получается, что внутри она должна менять стейт перед выполнением действия, а после - измененные значения заменять первоначальными. Тогда, например, вот этот код https://github.com/pf03/newBot/blob/2253dcc5eecee25dd292ad416111e4b361d74bdf/src/Messenger/Bot/VK/Internal.hs#L23-L28 превратится в этот

getInit :: Transformer Update.Init
getInit = 
  Log.withCustomSettings Color.Blue True "getInit" $ do
    let api = Bot.API Bot.Groups Bot.GetLongPollServer
    Log.writeSending
    query <- Query.getLongPollServerQuery
pf03 commented 3 years ago

Лучше сделать так, чтобы было сразу видно, что это транформер над IO, а также конкретизировать название, Transformer - слишком абстрактно :) Например type BotStateT = StateT BotState или type BotStateIO = StateT BotState IO

переименовал Transformer в BotState IO По остальным пунктам пока не успел переделать

pf03 commented 3 years ago

В этом модуле, мне кажется, перебор с тайп алиасами.

funcName и Enable удалил. ColorScheme переписал через data, чтобы исключить лишние цвета settingsEnable переименовал в settingsLogEnable, это локальное отключение лога, использовал при отладке

pf03 commented 3 years ago

Исправил все замечания