Епик Александр - Phase 1 #1

EvgeniyPukanovich · 2022-05-10T13:10:30Z

No description provided.

DenginRoman

Slick предоставляет возможности асинхронной работы с БД и здорово, что получилось их задействовать, а не приводить к синхронному виду через Await.result (исходя из закомментированного кода первая попытка видимо была такой).

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

Также ручной парсинг json'ов не оптимален, но уже с точки зрения кода. Парсинг это то, что должно быть определено в одном месте и происходить в одном месте (в момент получения данных). После парсинга мы должны работать уже с типизированными данными и ни в чем не сомневаться. Поэтому это тоже лучше переписать в более простой вариант - использованием DTO моделей (Data transfer object) и парсинг json'а средствами стандартных Reader'ов play'я

Ну и несколько замечаний по оформлению/подходу, безопасному извлечению данных и т.п.

Если что-то не понятно или есть возражения, можно оставлять тут комментарии и пинговать в чате в телеграмме

DenginRoman · 2022-05-12T13:22:51Z

app/controllers/JobAggregatorController.scala

+  def index(text: String, area: Int) = Action.async { implicit request: Request[AnyContent] =>
+
+    jobAggregatorService.parse(text, area).map(_ => Ok(""))
+      .recover(x => InternalServerError("Some exception has occurred"))


Сообщения вида "что-то пошло не так" малоинформативны. Лучше здесь описать что именно пошло не так. Как минимум в области видимости есть сама ошибка x, и из неё можно вытащить описание.

Кстати, имена переменных даже в лямбда функциях стоит делать максимально осмысленными, чтобы было проще читать и понимать код.

DenginRoman · 2022-05-12T13:37:57Z

app/model/Job.scala

+               title: Option[String],
+               requirement: Option[String],
+               responsibility: Option[String],
+               alternate_url: Option[String],


Лучше придерживаться единообразной стилистики в т.ч. в стиле написания составных слов. Или везде CamelCase'ом или везде snake_case'ом. Не обязательно имена полей case class'а должны совпадать с именами полей в БД - в JobTable всё равно идет маппинг

DenginRoman · 2022-05-12T13:41:05Z

app/scheduler/Task.scala

+    interval = Duration(interval).asInstanceOf[FiniteDuration]) { () =>
+    jobAggregatorService.parse(keyWords, cities)
+    println("Scheduled task executed")
+    //actorSystem.log.info("Executing something...")


Закоменнтированный код сохранять не стоит - он не несет смысловой нагрузки, но создает дополнительный шум

DenginRoman · 2022-05-12T13:46:19Z

app/scheduler/Task.scala

+    interval = Duration(interval).asInstanceOf[FiniteDuration]) { () =>
+    jobAggregatorService.parse(keyWords, cities)
+    println("Scheduled task executed")
+    //actorSystem.log.info("Executing something...")


Сообщение малоинформативно. Если будет несколько шедулеров, понять чье это сообщение, имя на руках только логи, не получится.

И кстати по поводу логгирования - просто выводить на экран сообщения менее эффективно, чем использовать специальные инструменты. Например, play.api.Logger, который позволяет присваивать сообщениям соответствующий уровень значимости (error, info, ...) и гибко конфигурировать источники, пункты назначения логгов (экран, файл, общее хранилище логов, ...), формат и полноту сообщений.

DenginRoman · 2022-05-12T14:00:01Z

app/scheduler/Task.scala

+                     configuration: Configuration,
+                     jobAggregatorService: JobAggregatorService)(implicit executionContext: ExecutionContext) {
+
+  val initialDelay: String = configuration.get[String]("initialDelay")


Здесь и далее при чтении конфига: а что будет, если в конфиге нет поля initialDelay? Возможно 2 вида поведения:

упасть с ошибкой инициализации (для этого сообщение должно быть понятным где и что пошло не так);

подставить значение по умолчанию (в этом случае должен лететь заметный warning, что используется значение по умолчанию)

DenginRoman · 2022-05-12T14:48:04Z

conf/application.conf

 slick.dbs.default.db.user="postgres"
 slick.dbs.default.db.password="12345678"

+//play.modules.enabled += "scheduler.TasksModule"


Закомментирование значение?

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

DenginRoman · 2022-05-12T14:58:44Z

app/scheduler/Task.scala

+  val keyWords: Seq[String] = configuration.get[Seq[String]]("keyWords")
+
+  actorSystem.scheduler.scheduleAtFixedRate(initialDelay = Duration(initialDelay).asInstanceOf[FiniteDuration],
+    interval = Duration(interval).asInstanceOf[FiniteDuration]) { () =>


Небезопасное приведение типов с помощью asInstanceOf - а если Duration(interval) не сможем скастить к нужному типу?

В таких случаях безопаснее применять паттерн матчинг (сопоставление по типу) и описывать 2 ветки - подошел / не подошел и, если не подошел, выводить ошибку с понятным текстом, или конструировать FiniteDuration на основе Duration(interval) явно

DenginRoman · 2022-05-12T15:01:07Z

app/service/JobAggregatorService.scala


-    def addJobTest(title: String): Future[Int] = {
-        db.run(jobTable += Job(UUID.randomUUID(), title))
+  val perPage: Int = 100


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

DenginRoman · 2022-05-12T15:01:57Z

app/service/JobAggregatorService.scala

+   * @param text ключевое слов
+   * @param area индекс региона
+   */
+  def parse(text: String, area: Int) = {


название метода parse не соответствует документации и выполняемым действиям (добавлению в БД)

DenginRoman · 2022-05-12T15:02:23Z

app/service/JobAggregatorService.scala

+   * @param keyWords ключевые слова
+   * @param areas индексы регионов
+   */
+  def parse(keyWords: Seq[String], areas: Seq[String]):Unit = {


аналогично нейминг

DenginRoman

Получилось упростить парсинг json'а за счет отдельных DTO - очень круто! В целом всё хорошо, за исключением некоторых моментов. Их поправить и будет аппрув.

Нужно привести в порядок чтение из конфига: обязательные поля не читать, как Option, сообщения формировать в момент чтения и присваивания значения.
Очень много выбрасывается исключений (комменты оставил не везде, но места легко найти по throw Exceptoin): Exception стоит завернуть в обертку, а данные, из-за которых происходит исключение и так в обертке - с ними стоит работать как с монадой, описывая цепочку преобразований через map/flatMap/... Иначе зачем вокруг все эти монады, обертки, типобезопасность, если есть шанс наткнуться на необработнное исключение (проект же будет развиваться, в коде нужно быть уверенным)

DenginRoman · 2022-05-30T09:17:43Z

app/controllers/JobAggregatorController.scala

-    jobAggregatorService.parse(text, area).map(_ => Ok(""))
-      .recover(x => InternalServerError("Some exception has occurred"))
+    jobAggregatorService.aggregateData(text, area).map(_ => Ok(""))
+      .recover(exception => InternalServerError("Following exception has occurred: " + exception.getMessage))


вместо конкатенации можно использовать интерполяцию:

InternalServerError(s"Following exception has occurred: ${exception.getMessage}")

DenginRoman · 2022-05-30T09:27:11Z

app/scheduler/Task.scala

-  val interval: String = configuration.get[String]("interval")
-  val cities: Seq[String] = configuration.get[Seq[String]]("cities")
-  val keyWords: Seq[String] = configuration.get[Seq[String]]("keyWords")
+  if (!configuration.has("initialDelay") || !configuration.has("interval")


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

DenginRoman · 2022-05-30T09:31:00Z

app/scheduler/Task.scala

-  val keyWords: Seq[String] = configuration.get[Seq[String]]("keyWords")
+  if (!configuration.has("initialDelay") || !configuration.has("interval")
+    || !configuration.has("cities") || !configuration.has("keyWords"))
+    throw new NoSuchFieldException("Configuration doesn't have some of these paths:" +


В общем случае кидать исключения не ФП стиль, т.к. исключения это сайд эффект. Альтернатива - конструирование объекта Constants (`Configuration',...), который может быть в одном из двух состояний - построился, и значит с параметрами всё в порядке, или не построился и хранит описание ошибок.

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

DenginRoman · 2022-05-30T09:34:59Z

app/scheduler/Task.scala

+    throw new NoSuchFieldException("Configuration doesn't have some of these paths:" +
+      " initialDelay, interval, cities, keyWords")
+
+  val initialDelay: Option[String] = configuration.getOptional[String]("initialDelay")


Зачем тип Option, если без поля initialDelayмы выкинем исключение (код выше)

Когда строковая константа встречается больше 1 раза, его выносят в именованную константу

Тут можно сделать одно из двух действий (зависит от задуманной логики):

при отсутствии значения в конфиге подставить значение по умолчанию:

val initialDelay: String = configuration.getOptional[String]("initialDelay").getOrElse("SomeDefaultValue")

2.при отсутствии значения в конфиге выкинуть исключение:

val initialDelay: String = configuration.getOptional[String]("initialDelay").getOrElse(sys.error("Error message"))

DenginRoman · 2022-05-30T09:36:35Z

app/scheduler/Task.scala

+  val keyWords: Option[Seq[String]] = configuration.getOptional[Seq[String]]("keyWords")
+
+  if (initialDelay.isEmpty || interval.isEmpty || cities.isEmpty || keyWords.isEmpty)
+    throw new ClassCastException("Some of these paths have wrong type: initialDelay, interval, cities, keyWords")


Снова общее сообщение об ошибке. Мало того, что не ясно, какой именно параметр имеет не верный формат, не ясно ещё и какой формат должен быть. Видимо, раз идет проверка на пустоту, должен быть Some. Но тогда следует сразу и читать параметр не как Option'ы

DenginRoman · 2022-05-30T09:37:15Z

app/scheduler/Task.scala

+  val interv: Try[FiniteDuration] = Try(Duration(interval.get).asInstanceOf[FiniteDuration])
+
+  if(initDelay.isFailure || interv.isFailure)
+    throw new ClassCastException("Initial delay or interval have wrong format")


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

DenginRoman · 2022-05-30T09:37:35Z

app/scheduler/Task.scala

+  val initDelay: Try[FiniteDuration] = Try(Duration(initialDelay.get).asInstanceOf[FiniteDuration])
+  val interv: Try[FiniteDuration] = Try(Duration(interval.get).asInstanceOf[FiniteDuration])
+
+  if(initDelay.isFailure || interv.isFailure)


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

DenginRoman · 2022-05-30T09:44:08Z

app/scheduler/Task.scala

+  actorSystem.scheduler.scheduleAtFixedRate(initialDelay = initDelay.get,
+    interval = interv.get) { () =>
+    jobAggregatorService.aggregateData(keyWords.get, cities.get)
+    Logger("play").info("Scheduled task executed")


Действительно ли это логгирование относится к play'ю? Обычно делают так, что каждый модуль имеет свой собственный логгер и через него логгирует сообщения. Так делают, чтобы в логах можно было быстро найти сообщения от соответствующего компонента.

Т.е. тут предлагаю сделать 2 действия:

Дать логгеру название соответствующее модулю

Вынести логгер в отдельную переменную:

val logger = Logger("RightName")

DenginRoman · 2022-05-30T09:44:47Z

app/service/JobAggregatorService.scala

+  val perPage: Int = configuration.getOptional[Int]("perPage") match {
+    case Some(value) => value
+    case None =>
+      Logger("play").warn("perPage is not configured. The default value(100) will be used")


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

DenginRoman · 2022-05-30T09:58:37Z

app/service/JobAggregatorService.scala

-      val jobs: ListBuffer[Job] = getJobsFromPage(firstResp, keyWord, areaText)
+      val pages = firstResp.asOpt[HolderDTO] match {
+        case Some(holder) => holder.pages
+        case None => throw new ClassCastException("json can't be parsed: " + firstResp.toString)


Здесь кидать исключение уже излишне. Раз pages Option, то с ним можно дальше работать как с монадой, описывая преобразования. Вероятно в конце будет результат вида Option[Future[..., а хотеться будет просто Future. Вот тут можно будет и развернуть Option, сделал getOrElse(Future.failed(new ClassCastException("json can't be parsed: " + firstResp.toString))). Здесь мы не выкинем исключение, а сгенерируем, завернем в обертку и понесем аккуратно наверх вызывающего кода

EvgeniyPukanovich added 2 commits May 9, 2022 16:58

two tables

b88ce97

phase 1

bf8de5a

DenginRoman suggested changes May 12, 2022

View reviewed changes

fixed problems

c01447f

DenginRoman suggested changes May 30, 2022

View reviewed changes

final changes

9bfb096

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Епик Александр - Phase 1 #1

Епик Александр - Phase 1 #1

EvgeniyPukanovich commented May 10, 2022

DenginRoman left a comment

DenginRoman May 12, 2022

DenginRoman May 12, 2022

DenginRoman May 12, 2022

DenginRoman May 12, 2022

DenginRoman May 12, 2022

DenginRoman May 12, 2022

EvgeniyPukanovich May 14, 2022

DenginRoman May 17, 2022

DenginRoman May 12, 2022

DenginRoman May 12, 2022

DenginRoman May 12, 2022

DenginRoman May 12, 2022

DenginRoman left a comment

DenginRoman May 30, 2022

DenginRoman May 30, 2022

DenginRoman May 30, 2022

DenginRoman May 30, 2022

DenginRoman May 30, 2022

DenginRoman May 30, 2022

DenginRoman May 30, 2022

DenginRoman May 30, 2022

DenginRoman May 30, 2022

DenginRoman May 30, 2022

Епик Александр - Phase 1 #1

Are you sure you want to change the base?

Епик Александр - Phase 1 #1

Conversation

EvgeniyPukanovich commented May 10, 2022

DenginRoman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenginRoman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment