blyscuit / Surveyer

0 stars 0 forks source link

Using Result<T, APIError> in network layer #2

Open edgarss opened 4 years ago

edgarss commented 4 years ago

If you changed the getSurveys completion handler from completionHandler: @escaping ([HotelModel]?, Error?) -> Void to completionHandler: @escaping Result<[HotelModel], Error> you would have a much nicer network layer and you could just return api.performRequest(with: configuration, completion: completion)(Alaomfire5 syntax)

Then in ViewModel you would have something like this

getSurveysRequest = service.getSurveys(page: page, perPage: perPage) { [weak delegate] result in
                switch result {
                case .success(let surveys):
                    delegate?.didGetSurveys(surveys)
                case .failure(let error):
                    delegate?.didFailToGetSurveys(error: error)
                }
            }
blyscuit commented 4 years ago

I'll try this after upgrading to AF5 #5

edgarss commented 4 years ago

syntax for Alamofire4 is very similar actually, I just noted that its AF5 to avoid confusion

blyscuit commented 4 years ago

Can you please clarify how would I convert Alamofire Result object to the pattern: Result<[HotelModel], Error>. I seem to not be able to find the method to turn Result<Any, AFError> to noted format.

I can see how this can reduce a lot of code from my Network Layer, thanks.

blyscuit commented 4 years ago

I forgot to mention that I tried .responseDecodable. Is this the right direction? by creating an object that can substitute for [HotelModel]?

blyscuit commented 4 years ago

I see, I can create new Result object using .success and .failure passing in the correct data.

blyscuit commented 4 years ago

I refracted the network layer as per https://github.com/blyscuit/Surveyer/commit/78c53f995cd7146ab1181e4af7b55b268421ba02. Still there's more to do, especially line 30, even thought the object is never nil (using ObjectMapper). I still don't think this is exactly what you're looking for and the code can be cleaner.

blyscuit commented 4 years ago

I read more on Alamofire5 Result and found that as of AF5, Result is subclass of Swift.Result, rather than Alamofire.Result, still, I don't think there's much difference to what goes in to completionHandler(). I refract the ViewModel more to make optional casting more bearable https://github.com/blyscuit/Surveyer/commit/5bc05aab4c16de0b7e6103aa86aaab9edfdd51e1. I change UnitTest to reflect the change https://github.com/blyscuit/Surveyer/commit/30a56a0688011046ef70ad1257e85037a1bdd5e9.

Still, I could use more feedback on this one. Thank you very much.

edgarss commented 4 years ago

I would have done it like this:

struct HotelModelNew: Decodable {
    let title: String?
    let description: String?
    let questions: [Questions]?
    let coverImageUrl: String?
}

struct Questions: Decodable {
    let test: String?
}

func getSurveys(page: Int = 0, perPage: Int = 10, completion: @escaping (Result<[HotelModelNew], AFError>) -> Void) -> Alamofire.Request {
    let url = baseURL + "surveys.json?page=\(page)&per_page=\(perPage)"
    let params = ["access_token": UserManager.getAccessToken()]

    return AF.request(url, method: .get, parameters: params).responseDecodable { (response: DataResponse<[HotelModelNew], AFError>) in
        switch response.result {
        case .success(let decoded):
            completion(.success(decoded))
        case .failure(let error):
            completion(.failure(error))
        }
    }
}
blyscuit commented 4 years ago

I see, if I were to do it with ObjectMapper, I'd need another lib on top.

// pod 'AlamofireObjectMapper'

AF.request(URL).responseObject { (response: DataResponse<[HotelModel], AFError>) in {
    // same block
}

At that point, I'd say it's too many libs for maintaining a good network service. Thank you for pointing that out.