Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exam application content #329

Merged
merged 29 commits into from
Jul 24, 2018
Merged

Exam application content #329

merged 29 commits into from
Jul 24, 2018

Conversation

ivan-magda
Copy link
Member

Задача: #APPS-1941

Описание:

  • структура для работы с графом: Implement graph data structure #318 #APPS-1956
  • загрузка JSON ресурса представления графа и его парсинг
  • заполнение графа полученными данными и отображение списка тем

@ivan-magda ivan-magda added this to the 1.64 milestone Jul 18, 2018
@ivan-magda ivan-magda self-assigned this Jul 18, 2018
@ivan-magda ivan-magda changed the title WIP: Application content Exam application content Jul 20, 2018
@ivan-magda ivan-magda requested review from kvld and Ostrenkiy July 20, 2018 11:00

var userRegistrationService: UserRegistrationService { get }

func userRegistrationService() -> UserRegistrationService
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А почему они стали методами, а не полями?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вот это кстати не знаю почему :)
Когда поле, то синтаксис при обращении намного дружелюбнее выглядит:

assemblyFactory.serviceAssembly().userRegistrationService()
assemblyFactory.serviceAssembly.userRegistrationService

Поправлю

import Foundation
import PromiseKit

private let url = URL(string: "https://www.dropbox.com/s/l8n1wny8qu0gbqt/example.json?dl=1")!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Константа вне класса, лучше сделать её приватной static в GraphServiceImpl (вроде только там она и используется)

Copy link
Contributor

@Ostrenkiy Ostrenkiy Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➕ к Владу, давайте такие константы вообще не использовать

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну это же все пока на первых порах, для проверки)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

final class GraphServiceImpl: GraphService {
func obtainGraph(_ completionHandler: @escaping (StepicResult<KnowledgeGraphPlainObject>) -> Void) {
firstly {
URLSession.shared.dataTask(.promise, with: url).validate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Предлагаю использовать Alamofire вместо того, чтобы дергать URLSession напрямую


protocol GraphService: class {
typealias Handler = (StepicResult<KnowledgeGraphPlainObject>) -> Void
func obtainGraph(_ completionHandler: @escaping Handler)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А какая мотивация использовать здесь коллбек? Может быть использовать промисы вместо этого?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мотивация была, чтобы не зависеть от PromiseKitа.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мне кажется, что так было бы удобнее, но смотри сам.
Кстати, Stepi_k_

extension Dictionary {
// swiftlint:disable:this implicit_getter
// Subscript is not mutating, get-only.
subscript(idx: Int) -> (key: Key, value: Value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачем этот subscript и каково его реальное применение?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это нужно для получения элемента словаря по индексу. Используется только в KnowledgeGraph.
Можно тогда перенести это расширение в файл реализации KnowledgeGraph и сделать его приватным?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, наверное лучше сделать приватным. Но вообще получать элемент словаря по целочисленному индексу – какая-то нестандартная идея 🤔Возможно, там где это используется, словарь не нужен

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Посмотрю, постараюсь без этого обойтись.


override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
let cell: TopicTableViewCell = tableView.dequeueReusableCell(for: indexPath)
presenter.configure(cell: cell, forRow: indexPath.row)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cell нужно конфигурировать во View слое.


override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
tableView.deselectRow(at: indexPath, animated: true)
presenter.didSelect(row: indexPath.row)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Думаю, Presenter не должен знать про устройство View. Предлагаю всё таки во View хранить данные для отображения (это должны быть обычные DTO объекты, которые можно передавать презентеру обратно).

protocol TopicsPresenter: class {
var numberOfTopics: Int { get }

func viewDidLoad()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нейминг методов здесь завязан на знаниях об устройстве View. Предлагаю использовать более абстрактные: refresh(), selectItem(_ item:), ...


import UIKit

final class TopicTableViewCell: UITableViewCell, TopicCellView {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Необязательно, кмк, накрывать ячейку протоколом.


tableView.registerNib(for: TopicTableViewCell.self)
presenter.viewDidLoad()
title = presenter.titleForScene()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Аналогично остальным замечаниям: это не обязанность презентера.

Copy link
Contributor

@kvld kvld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Всё ок, пусть ещё посмотрит Саша
cc @Ostrenkiy

private func viewTopicFromVertex<T>(_ vertex: KnowledgeGraphVertex<T>) -> TopicsViewData {
return TopicsViewData(
title: vertex.title,
onTap: { [weak self] in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не очень нравится идея биндиться через этот класс. Я бы лучше добавил в TopicsViewData айди и дергал презентер напрямую:
Вместо topics[indexPath.row].onTap() presenter.selectTopic(with: topics[indexPath.row]), а в презентере по полученному объекту ViewData что-то делал.
Но сказать почему так делать нельзя не могу, может быть @Ostrenkiy что-то добавит.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я написал через замыкание, т.к. лень было все явно прокидывать :)
Вариант с id правильнее и прозрачнее, поправлю.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А я, кстати, ничего против замыкания здесь не имею.
Единственная проблема тут, кажется, то, что это действительно непрозрачно и DTO перестает быть пассивным объектом.

@ivan-magda ivan-magda merged commit d4ee433 into dev Jul 24, 2018
@ivan-magda ivan-magda deleted the feature/app-content branch July 24, 2018 07:29
@Ostrenkiy Ostrenkiy mentioned this pull request Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants