Замечания по программе ParserRSS - 2 (Kotlin, CA)

[oMystique]

• [x] Следует выделить под формат данных статическую именованную константу с глобальной точкой доступа. Сейчас формат встречается в нескольких местах

  "EEE, dd MMM yyyy HH:mm:ss"

[genaku]

По Clean Architecture.

Интерактор и презентер должны быть независимы друг от друга. У них разные ответственности. Принцип такой: от UI через интерактор отдаются команды, данные, введенные пользователем, и прочее в UseCase. UseCase осуществляет обработку application logic и в качестве ответа отдает команды презентеру обновить UI.

UseCase при обработке application logic может обращаться к бизнес-модели (которая осуществляет бизнес-логику) и хранилищам данных. Для хранилищ данных в Clean Architecture применяется паттерн Repository.

Бизнес-логика - та логика, которая не зависит от приложения. Например, конвертация валюты. Application logic - это логика конкретного приложения: например, при добавлении новой rss-ленты осуществить открытие этой ленты.

[oMystique]

• [x] Дизайн:
• Следует убрать название приложения "MyRssParser"
• Сгруппировать все элементы (Add, Info, Delete) в рамках контекстного меню, убрав тем самым лишнюю кнопку с бара
• Ужать бар, имплементировав следующее поведение:

  На месте названия приложения располагаются табы с заголовками новостей Табы не перекрывают кнопку контекстного меню Табы скроллятся горизонтально Табы заезжают под кнопку вызова контекстного меню

[oMystique]

• [x] Следует написать нормальное наименование для тега

  Log.e("Tag", newCount.toString())

[oMystique]

• [x] Не очень удачно подобрано имя для класса. Следует избавиться от глагола и, возможно, использовать имя нарицательное в составе имени класса. К примеру, fetcher, puller...

  GetAllRssFromDbTask

[oMystique]

• [x] Можно сократить конструкцию

  !IMAGE_TYPES.contains(type)

  на

  type !in IMAGE_TYPES

[oMystique]

• [x] И вообще функцию можно превратить в это, при использовании конструкций котлина

  private fun parseArticleImage(item: Element, article: Article) {
      val resource = item.getChild(ARTICLE_IMG_NODE) ?: return
  
      val url = resource.getAttributeValue(ARTICLE_IMG_URL) ?: return
  
      if (resource.getAttributeValue(ARTICLE_IMG_TYPE) !in IMAGE_TYPES) {
          return
      }
  
      article.imageUrl = url
  }

из

    private fun parseArticleImage(item: Element, article: Article) {
        val resource = item.getChild(ARTICLE_IMG_NODE) ?: return

        val url = resource.getAttributeValue(ARTICLE_IMG_URL)
        val type = resource.getAttributeValue(ARTICLE_IMG_TYPE)

        if (url == null || !IMAGE_TYPES.contains(type)) {
            return
        }

        article.imageUrl = url
    }

[oMystique]

• [ ] При отсутствии пермишна на доступ к интернету, пользователю следует предлагать диалог с соответствующим текстом (HttpRequestHandler.State.PermissionDenied -> R.string.toast_internet_permission_denied) и с выбором варианта последующего действия:
  1. Открыть настройки, где можно выставить permission
  2. Закрыть диалог. (Диалог покажется вновь при попытке использовать функционал с интернет соединением)

[oMystique]

• [x] iconsQueue.contains(id)-> id !in iconsQueue

А так же...

[oMystique]

• [x] Странное название класса, лучше переформулировать

  GetImageFromNetForFeedItem

[oMystique]

• [x] Следует пометить комментарием, что имплементация отсутствует не просто так в теле данного метода: либо она будет ('TODO'), либо ее не будет вообще ('Not impl')

  fun onOpenOriginal() {
  
  }

[oMystique]

• [x] Следует имена данных приватных переменных написать с префиксом m и без капса

      private lateinit var CONTEXT: WeakReference<Context>
      private lateinit var BOX_STORE: BoxStore

[oMystique]

• [x] Нет смысла ссылать слабой ссылкой на контекст:

      private lateinit var CONTEXT: WeakReference<Context>

[oMystique]

• [x] Для приватных переменных внутри класса используем префикс m

[oMystique]

• [x] Для котлина лучше создать ветку master-kotlin

[genaku]

Непонятно зачем интеракторы в каждом методе создают новые UseCase. Сейчас у тебя каждый UseCase - это отдельный async task - сущность из Android framework. UseCase - это application logic, она не должна зависеть от платформы. Слой UseCase должен быть реализован так, чтобы его можно было легко использовать на любой платформе, где поддерживается язык, на которым эти классы написаны. В чистом виде от платформы зависят только самые внешние слои: View и Repository

[oMystique]

• [ ] Тесты для UseCases лучше сделать Unit-тестами (без вызова Roboleptic)

[oMystique]

• [ ] urlsWithRelativePartsEquals - тест не проходит

[genaku]

Сейчас у тебя View model Rss знает напрямую о DB model Rss. Возникла необходимость поменять БД на другую. Эту задачу поручили делать другой компании. Они написали новый модуль, отвечающий за хранение данных. Но они не знали, что у тебя ViewModel использует классы из БД. Мы подцепили их модуль и приложение сломалось. Кто виноват?