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

Fetching data from api #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ghost
Copy link

@ghost ghost commented Feb 13, 2020

No description provided.

Copy link
Contributor

@mvn-tienle-dn mvn-tienle-dn left a comment

Choose a reason for hiding this comment

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

Nhận xét chung thì OKE về yêu cầu. Nhưng vẫn chưa mãn nguyện lắm. Có các vấn đề còn tồn đọng như sau:

  • schedule cho fetchData & receive ở đâu, mình thiếu quản lý queue
  • phân biệt giữa RunLoop và Queue
  • MVVM vs. Combine

request.httpBody = target.parameter?.data()
}

return URLSession.shared.dataTaskPublisher(for: request).tryMap { (data, response) -> Data in
Copy link
Contributor

Choose a reason for hiding this comment

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

Thiếu receive lên global queue để chạy ở background.

return data
}
.decode(type: T.self, decoder: JSONDecoder())
.mapError({ error -> Network.Error in
Copy link
Contributor

Choose a reason for hiding this comment

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

switch case các error. Vì có nhiều loại error trả về, có thể do request hay parse hoặc chung chung

var parameter: Parameters? { get }
}

enum Target: TargetType {
Copy link
Contributor

Choose a reason for hiding this comment

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

Đổi tên thành Router hay gì đó khác, target trùng với UIKit cho các UI Control events

@IBOutlet private weak var tableView: UITableView!
let viewModel = ViewModel()
var cancellable: [AnyCancellable] = []
private var developers: [Developer] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Em đang dùng MVVM thì để array ở đây không hơp lý rồi


@IBOutlet private weak var tableView: UITableView!
let viewModel = ViewModel()
var cancellable: [AnyCancellable] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Phần MVVM anh chưa thử ntn, tạm thời thì oke khoản này

import Foundation
import Combine

final class ViewModel {
Copy link
Contributor

Choose a reason for hiding this comment

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

ViewModel kế thừa lài ObservableObject và thuộc tính của nó muốn cho bên VC biết thì thêm @Published vào trước. Sau khi fetchData thành công thì thay vì dùng sink thì assign trực tiếp tới property của VM.

Bên VC cần subscription tới property của MV là ổn rồi.

@mvn-tienle-dn
Copy link
Contributor

Ví dụ phần schedule

//Khai báo

private let decoder = JSONDecoder()
private let apiQueue = DispatchQueue(label: "API", qos: .default, attributes: .concurrent)

//Request

  func story(id: Int) -> AnyPublisher<Story, Error> {
    URLSession.shared
      .dataTaskPublisher(for: EndPoint.story(id).url)
      .receive(on: apiQueue)
      .map(\.data)
      .decode(type: Story.self, decoder: decoder)
      .catch { _ in Empty<Story, Error>() }
      .eraseToAnyPublisher()
  }

Sử dụng

fetchData()
      .receive(on: DispatchQueue.main)
      .sink(receiveCompletion: { completion in
        if case .failure(let error) = completion {
          print("❌ :  \(error.localizedDescription)")
        }
        }) { musics in
          self.musics = musics
        }
      .store(in: &subscriptions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant