Ревью - Githubissues

Комментарии. Есть хорошее правило понятного кода: понтяному коду не нужны комментарии, в нем и так понятно что просходит :) Я разберу на примере: https://github.com/wixe/vktgbot.hs/blob/master/src/Telegram/User.hs#L12 - в этом случае наличие комментария точно избыточно, по названию понятно о чем идет речь. Если ты хочешь дать понять, что речь может идти о работе с ботом и юзером, ты мог бы назвать сам тип не Json, а, к примеру, UserBotInfo, эта идея относится ко всем типам с именем Json. А вот здесь https://github.com/wixe/vktgbot.hs/blob/master/src/Bot.hs#L55 напротив стоило бы пояснить зачем ты переходишь от ленивых вычислений к активным, и тут бы действительно не помешал комментарий. Ещё добавлю, что комментарии лучше писать либо сверху над чем-то, либо, в случае с полем, на определенном удаление:
```
data Type = Type {
field1 :: Type1                          -- Comment
field2 :: Type2                          -- Comment
}
```
Тогда это становится проще читать.
Модульность и стуктура проекта. Есть несколько файлов, к примеру, вот этот https://github.com/wixe/vktgbot.hs/blob/master/src/Bot.hs сейчас его сложно читать, там идут в перемешку instance, функции и типы. Не бойся делать много папок и файлом, напротив, это хорошо и улучшает читабельность В идеале каждый тип или группа связанных типов должны быть в своей папке и в своем файле. Все функции которые работают с этим типом должны тоже должны лежать вместе, но не обязательно в том же файле, что и тип, тут уже смотри по ситуации. Названия модулей и папок должны давать понять о чем идет речь и что лежит внутри модуля/папки
Функции и типы Если идет несколько вложенных case of или довольно большое тело функции, по возможности старайся разбивать это на смысловые части, выносить что-то в блок where, использовать let, выносить функцию наружу. К примеру, такая лямбда выглядит очень сложной: https://github.com/wixe/vktgbot.hs/blob/master/src/Bot.hs#L194 Не забывай везде писать сигнатуры: https://github.com/wixe/vktgbot.hs/blob/master/src/Options.hs#L45
Тесты и ошибки. Здесь все те же рекомендации, что и выше, но добавлю, что тестов явно должно быть больше, чтобы они по максимум покрывали всю логику. Очень советую использовать Handle Pattern: https://jaspervdj.be/posts/2018-03-08-handle-pattern.html https://www.schoolofhaskell.com/user/meiersi/the-service-pattern Он поможет тебе организовать логику и организовать тесты. Я не особо увидел, чтобы ты где-то ловил ошибки и где-то их бросал, вот что советую почитать https://hackage.haskell.org/package/base-4.14.0.0/docs/Control-Exception.html Постарайся ловить ошибки везде где это возможно, а после, написать тесты для каждой такой ошибки, где рассматривается случай что ошибка происходит и случай что все работает корректно

https://github.com/wixe/vktgbot.hs/blob/master/src/Telegram/User.hs#L15 - в этом случае наличие комментария точно избыточно, по названию понятно о чем идет речь. Если ты хочешь дать понять, что речь может идти о работе с ботом и юзером, ты мог бы назвать сам тип не Json, а, к примеру, UserBotInfo, эта идея относится ко всем типам с именем Json.

Все типы с именем Json в директориях Telegram и Vk это объекты, определяемые конкретным сервисом. Они предназначены для использования с квалифицированным импортом, например: Message.Json. Такой нейминг кажется мне обоснованным

А вот здесь https://github.com/wixe/vktgbot.hs/blob/master/src/BotAPI.hs#L16 напротив стоило бы пояснить зачем ты переходишь от ленивых вычислений к активным, и тут бы действительно не помешал комментарий.

Не уверен что стоит написать, "потому что это хендлер", "потому что точно будет посчитано"? Такую практику подсмотрел в этой статье, но почему именно там используются бенги не написано

Не забывай везде писать сигнатуры: https://github.com/wixe/vktgbot.hs/blob/master/src/Options.hs#L74

Прям везде-везде для каждой функции? Не много ли визуального шума получится? И как же type inference?

Я не особо увидел, чтобы ты где-то ловил ошибки и где-то их бросал

Сделал часть логики через ExceptT Text IO

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

Не совсем понятно что имеется ввиду, большая часть кода живет в IO и его тестировать в отдельности (аля юнит тест) не представляется возможным (да еще и зависимость от внешних ресурсов), а если вместе то это уже integration тест получится. Фримонадную логику самого бота (взаимодействие с пользователем через чат) я вроде протестировал с помощью тестового интерпретатора. Еще можно, наверно, на каждый парсер по паре тестов сделать, но это мне кажется излишним (или нет?). Для совсем простых функций тестирующая функция будет реимплементировать тестируемую функцию, что, как мне кажется, мало имеет смысла

Остальные замечания постарался учесть и применить

Все типы с именем Json в директориях Telegram и Vk это объекты, определяемые конкретным сервисом. Они предназначены для использования с квалифицированным импортом, например: Message.Json. Такой нейминг кажется мне обоснованным

Окей, в таком виде это уже звучит логично, оставь так

Не уверен что стоит написать, "потому что это хендлер", "потому что точно будет посчитано"? Такую практику подсмотрел в этой статье, но почему именно там используются бенги не написано

Если ты не понимаешь зачем использовать фичу, то лучше её не использовать. Либо объясни, что без неё пойдет не так

Прям везде-везде для каждой функции? Не много ли визуального шума получится? И как же type inference?

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

Не совсем понятно что имеется ввиду, большая часть кода живет в IO и его тестировать в отдельности (аля юнит тест) не представляется возможным (да еще и зависимость от внешних ресурсов), а если вместе то это уже integration тест получится. Фримонадную логику самого бота (взаимодействие с пользователем через чат) я вроде протестировал с помощью тестового интерпретатора. Еще можно, наверно, на каждый парсер по паре тестов сделать, но это мне кажется излишним (или нет?). Для совсем простых функций тестирующая функция будет реимплементировать тестируемую функцию, что, как мне кажется, мало имеет смысла

Окей, как это работает, у нас есть какая-нибудь функция, которая работает с IO и ищет что-нибудь, к примеру

data Handle = { 
    findSmthById :: Text -> IO (Maybe Smth.Smth)
    ...
}

в коде, где она используется ты проверяешь её результат

smth <- findSmthById smthId
maybe throwSmthNotFound pure smth

Чтобы протестировать работу этой функции ты пишешь заглушку для неё, когда она работает корректно:

handle :: UseCase.Handle
handle = {
  findSmthById = \smthId -> do
      pure $ Just smth

И вариант когда функция должна упасть упасть

badHandle = handle {
  findSmthById = \smthId -> do
      pure Nothing
}

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

И пока все ещё есть вопросы к структуре проекта, ты понял, что ожидается увидеть, может есть вопросы? Потому что эти файлы сейчас выглядят очень объемными https://github.com/wixe/vktgbot.hs/blob/master/src/Bot.hs https://github.com/wixe/vktgbot.hs/blob/master/src/Vk.hs К примеру, в первом файле я бы вынес тип BotOptions в отельный файл и связанную с ним функцию mkBotOptions Во втором аналогично с VkPollResult и parseUpdate. Поработай ещё над этим пожалуйста

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

Сделал подобные тесты для Bot.interpret, но без контр кейсов, потому что единственная тестируемая функция это BotAPI.apiSendMessage и единственный случай ее фейла это если реквест не удался. BotAPI.apiGetMessages вызывается только из Bot.runBot, которую не потестить из-за ее IOшности.

И пока все ещё есть вопросы к структуре проекта, ты понял, что ожидается увидеть, может есть вопросы?

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

Остальные замечания постарался учесть и применить

sirewix / vktgbot.hs

Ревью #2