Мазырина Алена - Этап 1 #2

alenmaz · 2022-05-12T16:32:19Z

No description provided.

DenginRoman

Многие функции, работающие со фьючами, возвращают фьючи, а преобразования описаны с помощью map, flatMap - круто! Применили асинхронный подход! Но некоторые функции асинхронный код превращают в синхронный через Await.result, что сводит на нет все плюсы асинхронного кода и усилия, потраченные на его написание. Нужно везде сохранить асинхронность. Я комменты на каждом месте не оставлял, но их легко найти по Await.result

Парсинг Json'ов кажется громоздким и его можно упростить. Достаточно описать модели данных, которые приходят из вне, описать для них ридеры, а дальше json парсить сразу в модель с помощью json.asOpt[JobDTO]. При этом парсинг стоит проводить максимально близко к точки приёма данных из внешнего источника, чтобы после этого быть уверенным, что мы работает с конкретными типами и валидными данными.

Модели для БД и внешнего взаимодействия стоит разделять. Это способствует более лёгкой поддержки приложения в будущем

UPD
4.
Логгирование через print нужно заменить на логгирование через, например, play.api.Logger.

def foo(x: Data)  {
  Try(doSomethingWithX(x)) match {
    case Success(res) => logger.info("Успех")
    case Failure(e) => logger.error("Ошибка")
  }
}

DenginRoman · 2022-05-14T09:50:00Z

app/controllers/JobAggregatorController.scala

+
+  def loadJobs(tag: Option[String], area: Option[String]) = Action {
+    implicit request: Request[AnyContent] => {
+          Try(Await.result(jobAggregatorService.processJobResponse(jobAggregatorService.buildJobRequest(tag, area)), Duration.Inf)) match {


Преобразование асинхронного кода в синхронный с помощью Await.result и тем самым блокировка исходного потока. Т.е. исходный поток будет вынужден ждать, пока завершится запрос к БД,(а выставляя таймаут Duratino.Inf есть вероятность ждать бесконечно) вместо того, чтобы выполнять какую-то полезную нагрузку. А у слика для работы с БД пул потоков отдельный.

Если Await.result тут использовался для того, чтобы развернуть результат фьючи для помещения его в Try, то лучше использовать стандартные комбинаторы монады (map, flatMap, ...) или специальные комбинаторы фьючи (transform, recover, ...)

Если же Await.restul использовался из-за того, что Action требует Result, а не фьючу, то стоит использовать асинхронный вариант Action.async, который ждет функцию, возвращающую фьючу:

def loadJobs(tag: Option[String], area: Option[String]) = Action.async { implicit request => Future(Ok("ok")) }

Ну и везде, где используется Await.result его следует убрать, заменяя раскрытие монады Future и манипуляции с полученным значением на цепочки мандных комбинаторов, описывающих преобразование данных. Во-первых это более ФП подход, а во-вторых более эффективный с точки зрения производительности и использования ресурсов.

Я несколько переделала код, и у меня появилось несколько вопросов.

def loadJobs(keyword: Option[String], area: Option[String]) = Action.async { implicit request: Request[AnyContent] => { jobRequestService.processJobResponse(jobRequestService.buildJobRequest(area, keyword)).flatMap { case Some(value) => Await.result(jobAggregatorService.addJobs(value, area, keyword), Duration.Inf) Future(Ok(s"Loading of jobs with tags (${keyword.getOrElse("Empty")}, ${area.getOrElse("Empty")}) done")) } } }

Мне не очень понятно, как можно было бы избавиться от Await.Result здесь. Если не дожидаться завершения всех фьюч, приложение заканчивает работу раньше, чем они все добавятся в БД, и в итоге в БД данные не добавляются.

def buildJobRequest(area: Option[String], keyword: Option[String]): WSRequest = { logger.info("Building request...") logger.info(s"Keyword: ${keyword.getOrElse("none")}") logger.info(s"Area: ${area.getOrElse("none")}") ws.url("https://api.hh.ru/vacancies").addQueryStringParameters { keyword match { case Some(value) => "text" -> value } area match { case Some(value) => "area" -> Await.result(getAreaId(value), Duration.Inf).get } } }

private def getAreaId(name: String): Future[Option[String]] = ws.url("https://api.hh.ru/areas").get().map { response => Json.parse(response.body) match { case result: JsValue => result.asOpt[List[Area]] match { case Some(areas) => areas.foldLeft(List[Area]())((accumulator, area) => accumulator ++ area.areas.getOrElse(List[Area]())) .find(_.name == name).map(_.id) case None => logger.error(s"Parsing of areas failed, ${response.body}") None } case _ => logger.error(s"Couldn't parse areas json, ${response.body}") None } }

Аналогично возникла проблема при переписывании request'а - нужно дождаться списка Area, чтобы вытащить оттуда айдишник.

Немного оффтопик

Исходный код в комментариях можно раскрашивать в соответствии с синтаксисом скалы (или в соответствии с другим языком) :) Для этого нужно указать что за язык в блоке кода используется:

Следующий блок

```scala
def someAction(x: Foo): Bar = ???
```

превратится в раскрашенный код:

def someAction(x: Foo): Bar = ???

1. По поводу первого блока loadJobs:
Если абстрагировано взглянуть на действия

Await.result(jobAggregatorService.addJobs(value, area, keyword), Duration.Inf) Future(Ok(s"Loading of jobs with tags (${keyword.getOrElse("Empty")}, ${area.getOrElse("Empty")}) done"))

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

jobAggregatorService.addJobs(value, area, keyword).get Option((Ok(s"Loading of jobs with tags (${keyword.getOrElse("Empty")}, ${area.getOrElse("Empty")}) done"))

Здесь можно применить стандартные комбинаторы монады, которые описывают преобразование данных без раскрытия монады. Т.е. map и flatMap. Для Future эти комбинаторы опишут последовательное выполнение. Т.е. то, что внутри map будет выполняться только после того, как завершаться действия исходной фьючи. НО в отличии от Await.result мы уже не будет заставлять основной поток ждать окончания работы этой фьючи. Основной поток сконструировал нам фьючу, описанную цепочкой map'ов , flatMap'ов и запустил её (возможно в другом контексте) и пошел выполнять другую полезную работу.

2. блок buildJobRequest
Если мы хотим, чтобы наш код был асинхронным, нам придется пробрасывать асинхронность (фьючи) по всему стеку вызову. Т.е. если у нас в функции вызывается какая-то функция тип результата которой фьюча, то и тип результата нашей функции должен быть фьюча. Без этого асинхронность нарушится. Т.е. тут можно тип результата функции buildJobRequest сделать не WSRequest, а Future[WSRequest]. При этом нам сначала придется вычислить зависимую фьючу getAreaId и через map/flatMap сконструировать фьючу с внутрянкой WSRequst на основе areaId и вернуть её в качестве результата

НО
Исходя из названия и применения эта функция просто конструирует WSRequest и никаких внешний доп. действий (ходить во внешний сервис) не должна. Поэтому можно немного изменить сигнатуру и в качестве аргумента принимать не area, а areaId. А вот вычислять areaId по area (ходить во внешний сервис за ним) уже в том методе, где будет вызываться buildJobRequest и выполняться ws.get.

DenginRoman · 2022-05-14T09:57:28Z

app/controllers/JobAggregatorController.scala

+          Try(Await.result(jobAggregatorService.processJobResponse(jobAggregatorService.buildJobRequest(tag, area)), Duration.Inf)) match {
+            case Success(value) => value match {
+              case Some(list) => for(task <- jobAggregatorService.addJobs(list, area.getOrElse(""), tag.getOrElse(""))) Await.result(task, Duration.Inf)
+              case None => print("Couldn't get list of jobs")


Логгирование следует осуществлять специальными средствами (например, play.api.Logger), которые:

умеют присваивать уровень критичности сообщениям

инкапсулируя в себе логику вывода сообщения могут гибко настраиваться - в каком формате вывести сообщения (например, можно к сообщению добавить время, не меняя исходный код, а меняя только конфиг), куда вывести (файл/экран/общее хранилище логгов) и т.п.

Обычные print'ы же достаточно сложно потом будет сопровождать, если будет необхдимо как-то изменить логику логгирования

DenginRoman · 2022-05-14T09:59:16Z

app/controllers/JobAggregatorController.scala

+              case Some(list) => for(task <- jobAggregatorService.addJobs(list, area.getOrElse(""), tag.getOrElse(""))) Await.result(task, Duration.Inf)
+              case None => print("Couldn't get list of jobs")
+            }
+            case Failure(_) => print("Failed job response processing")


В сообщения об ошибках лучше добавлять подробности, т.к. сообщения вида "что-то пошло не так" не дают информации для решения проблемы:

case Failure(e) => print(s"Failed job response processing: ${e.getMessage}")

DenginRoman · 2022-05-14T10:07:06Z

app/model/Area.scala

+import play.api.libs.json._
+import play.api.libs.functional.syntax._
+
+case class Area(id: String, name: String, children: Option[JsValue])


В данном кейсклассе смешиваются распарсенные данные (id, name) и нераспарсенные, но представляющие собой какую-то структуру, описанную в api. Такое совмещение усложнит работу с полученными данными.

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

case class Area(id: String, name: String, children: Option[Seq[child]]) case class Child(<поля дочернего элемента>) object Area { implicit val areaReads: Reads[Area] = Json.reads[Area] //кстати, для парсинга простых данных и составленных из них кейс классов можно использовать стандартный ридер } object Child { implicit val childReads: Reads[Child] = Json.reads[Child] }

DenginRoman · 2022-05-14T10:09:43Z

app/model/JobRequest.scala

@@ -0,0 +1,3 @@
+package model
+
+case class JobRequest(jobid: String, city: String, keyword: String)


составные слова лучше писать используя визуальные разделения слов: camelCase'ом или snake_case 'ом, но единообразно

DenginRoman · 2022-05-14T10:50:51Z

app/model/Job.scala

+      ((JsPath \\ "requirement").read[String] or Reads.pure("")) and
+      ((JsPath \\ "responsibility").read[String] or Reads.pure("")) and
+      ((JsPath \ "salary" \ "from").read[Int] or Reads.pure(0)) and
+      ((JsPath \ "salary" \ "to").read[Int] or Reads.pure(0)) and


А точно ли нужны деволтные значения

DenginRoman · 2022-05-14T10:53:33Z

app/model/db/JobRequestTable.scala

+  def jobid = column[String]("jobid", O.PrimaryKey)
+  def city = column[String]("city")
+  def keyword = column[String]("keyword")


составные слова написаны слитно и не понято

DenginRoman · 2022-05-14T10:53:59Z

app/model/db/JobTable.scala

+    def requirements = column[String]("requirements")
+    def responsibility = column[String]("responsibility")
+    def salaryFrom = column[Int]("salaryfrom")
+    def salaryTo = column[Int]("salaryto")
+    def salaryCurr = column[String]("salarycurr")
+    def url = column[String]("url")


составные слова написаны слитно и не понятно

DenginRoman · 2022-05-14T10:55:51Z

app/service/JobAggregatorService.scala


-    def addJobTest(title: String): Future[Int] = {
-        db.run(jobTable += Job(UUID.randomUUID(), title))
+    def processJobResponse(request: WSRequest): Future[Option[List[Job]]] = {


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

DenginRoman · 2022-05-14T10:57:03Z

conf/evolutions/default/1.sql

+CREATE TABLE jobtable (
+    id VARCHAR(32),
+    title VARCHAR(512),
+    requirements TEXT,
+    responsibility TEXT,
+    salaryFrom INT,
+    salaryTo INT,
+    salaryCurr VARCHAR(8),
+    url TEXT,
+    PRIMARY KEY (id)
+);
+
+CREATE TABLE jobrequesttable (
+    jobid VARCHAR(32),
+    city VARCHAR(64),
+    keyword TEXT,
+    FOREIGN KEY(jobid) REFERENCES jobtable(id)
+)


слитно написанные составные слова

DenginRoman · 2022-05-16T14:20:08Z

@alenmaz , я сейчас только заметил, что в коде несколько раз встречается генерация значения null. Это опасно, т.к. наличие null хотя бы в одном месте программы заставляет проверять на null всё. Без проверок есть риск наткнутся на необработанное исключение NullPointerExecption. Если где-то требуется описание отсутствия чего-то, стоит использовать значение None монады Option и работать с ним с помощью map'ов flatMap'ов. Это безопаснее и надежнее, т.к. на уровне типа мы видим что есть возможность отсутствия значения и соответствующим образом обработаем.

DenginRoman

Супер! Получилось перейти на асинхронный подход! Очень круто, что нигде нет выброса исключения и идет работа с мондами через комбинаторы / паттерн матчинг! Истинный ФП стиль :)

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

DenginRoman · 2022-05-30T11:13:44Z

app/controllers/JobAggregatorController.scala

-      }
+      jobRequestService.getAreaId(area).flatMap(areaId =>
+        jobRequestService.processJobResponse(jobRequestService.buildJobRequest(areaId, keyword)).flatMap {
+          case Some(value) => jobAggregatorService.addJobs(value.map(dto => dto : Job), area, keyword).flatMap(_ => Future(Ok(s"Loading of jobs with tags (${keyword.getOrElse("Empty")}, ${area.getOrElse("Empty")}) done")))


Future.flatMap(x => Future(Ok(x)) то же самое, что и Future.map(x => Ok(x)) только менее многословно ;)

DenginRoman · 2022-05-30T11:14:37Z

app/model/Job.scala

+                  title: String,
+                  requirements: Option[String],
+                  responsibility: Option[String],
+                  salaryFrom: Option[Int],
+                  salaryTo: Option[Int],
+                  salaryCurrency: Option[String],
+                  url: String)


поехали отступы, минор

DenginRoman · 2022-05-30T11:14:49Z

app/model/Job.scala

+               title: String,
+               requirements: Option[String],
+               responsibility: Option[String],
+               salaryFrom: Option[Int],
+               salaryTo: Option[Int],
+               salaryCurrency: Option[String],
+               url: String)


снова отступы, минор

DenginRoman · 2022-05-30T11:16:42Z

app/model/Job.scala

 object Job {
-  implicit val jobReads: Reads[Job] = (
-      (JsPath \ "id").read[String] and
+  implicit  def jobToJobDTO(job: Job): JobDTO = JobDTO(


Вот насчет имплиситного преобразования Job в JobDTO я бы задумался. Неявные манипуляции без особой причины усложняют чтение кода. И кстати, зачем надо уметь Job приводить к JobDTO (модель из БД к модели из стороннего сервера, на который мы ничего не шлем, а наоборот из него читаем и кладем в БД)

DenginRoman · 2022-05-30T11:17:07Z

app/model/Job.scala

+}
+
+object JobDTO {
+  implicit def jobDTOtoJob(jobDTO: JobDTO): Job = Job(


аналогично имплиситное преобразование, точно ли нужно?

DenginRoman · 2022-05-30T11:43:42Z

app/schedulers/SchedulerActor.scala

-class SchedulerActor @Inject()(jobAggregatorService: JobAggregatorService)(implicit ec: ExecutionContext) extends Actor {
+class SchedulerActor @Inject()(jobRequestService: JobRequestService, jobAggregatorService: JobAggregatorService)(implicit ec: ExecutionContext)
+  extends Actor {
+  val logger = Logger("debug")


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

DenginRoman · 2022-05-30T11:44:48Z

app/schedulers/SchedulerActor.scala

+          jobRequestService.processJobResponse(jobRequestService.buildJobRequest(areaId, Some(keyword))).flatMap({
+            case Some(value) => jobAggregatorService.addJobs(value.map(dto => dto : Job), Some(area), Some(keyword)).flatMap(_ => Future())
+            case None => logger.error(s"Couldn't get list of jobs for params $area $keyword\n")
+              Future()


Посто Future? Без всего? Если нужен тип Future[Unit], то есть спец метод Future.unit

DenginRoman · 2022-05-30T11:56:26Z

app/service/JobAggregatorService.scala

-        for(job <- jobs)
-            yield addJob(job, area, keyword)
+    def addJobs(jobs: Seq[Job], area: Option[String], keyword: Option[String]): Future[Seq[Unit]] = {
+        Future.traverse(jobs)(job => addJob(job, area, keyword))


Если это была попытка избавиться от запуска всего списка задач параллельно, то Future.traverse наоборот параллельно запускает задачи. Разве что тип результата теперь другой. Чтобы описать последовательный запуск фьюч, придется использовать комбинаторы flatMap

DenginRoman · 2022-05-30T12:00:02Z

app/service/JobRequestService.scala

+    })
+  }
+
+  def getAreaId(name: Option[String]): Future[Option[String]] = {


Большая вложенность паттерн матчинга, она точно нужна? Если да, возможно стоит внутрянку вынести в отдельный метод

DenginRoman · 2022-05-30T12:04:09Z

app/schedulers/JobAggregatorScheduler.scala

+    Seq[String]()
+  })
+
+  if(doRun.getOrElse {


Начиная с этого момента используется getOrElse для применения значения по умолчанию при отсутствии параметра в конфиге. Лучше тогда применить это в сам момент чтения параметра и типы сделать не Option[Boolean], а просто Boolean (ну и с другими параметрами тоже), чтобы применение значения по умолчанию было в одном единственном месте максимально близко к получению значения

alenmaz and others added 5 commits May 5, 2022 18:48

scheduler

ca5fad8

fixed db

c238ba6

more tags

e725419

added area request

6e80769

database views

f10819e

DenginRoman suggested changes May 14, 2022

View reviewed changes

alenmaz added 6 commits May 21, 2022 21:55

fixed (no JobDTO)

a96c362

fixed area bug

6782333

fixed area bug 2

05c218a

.

25c9b1d

fixed tags

e0cdd23

jobDTO

d528459

DenginRoman suggested changes May 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Мазырина Алена - Этап 1 #2

Мазырина Алена - Этап 1 #2

alenmaz commented May 12, 2022

DenginRoman left a comment •

edited

Loading

DenginRoman May 14, 2022

alenmaz May 15, 2022 •

edited

Loading

DenginRoman May 16, 2022

DenginRoman May 14, 2022

DenginRoman May 14, 2022

DenginRoman May 14, 2022

DenginRoman May 14, 2022

DenginRoman May 14, 2022

DenginRoman May 14, 2022

DenginRoman May 14, 2022

DenginRoman May 14, 2022

DenginRoman May 14, 2022

DenginRoman commented May 16, 2022 •

edited

Loading

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

		@@ -0,0 +1,3 @@
		package model

		case class JobRequest(jobid: String, city: String, keyword: String)

Мазырина Алена - Этап 1 #2

Are you sure you want to change the base?

Мазырина Алена - Этап 1 #2

Conversation

alenmaz commented May 12, 2022

DenginRoman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenmaz May 15, 2022 • edited Loading

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 commented May 16, 2022 • edited Loading

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

DenginRoman left a comment •

edited

Loading

alenmaz May 15, 2022 •

edited

Loading

DenginRoman commented May 16, 2022 •

edited

Loading