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

Feature [RM80] Character Repository #81

Merged
merged 6 commits into from
Aug 13, 2021

Conversation

swg99
Copy link
Contributor

@swg99 swg99 commented Aug 11, 2021

https://github.com/orgs/novoda/projects/1#card-66585869
Built a repository which provides each page of characters from the api.

The repository provides the original page of characters and each one after.
Copy link
Contributor

@wrumble wrumble left a comment

Choose a reason for hiding this comment

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

Looking way better 💪 🎉

@@ -8,6 +8,19 @@

import Foundation


struct CharactersList: Decodable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Id probably update the naming here as it isnt a list. Maybe just CharactersResponse is my usual go to for this kind of base level json response.


struct CharactersList: Decodable {
let info: CharacterListInfo
let results: [Character]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd utilise the CodingKeys enum here so you can give the property a better name like characters. See Mapping different key names section

let results: [Character]
}

struct CharacterListInfo: Decodable {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could then become CharacterResponseInfo if you go with CharactersResponse above

Comment on lines 16 to 17
if let nextURLString = characterList.info.next {
self.characterPageURL = URL(string: nextURLString)!
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add another let to this if let to remove the ! in URL(string: nextURLString)!

Comment on lines 18 to 22
if requestError != nil {
DispatchQueue.main.async {
error(requestError)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this here if its being done in the else on line 37? Also why is requestError not being checked for nil in that else?

Comment on lines 11 to 13
protocol RickAndMortyServiceProtocol {
func fetchData<T:Decodable>(url: URL, success: @escaping (T) -> (), error: @escaping (Error?) -> ())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@swg99 swg99 linked an issue Aug 12, 2021 that may be closed by this pull request
@swg99 swg99 merged commit cbab52a into scottie-main Aug 13, 2021
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.

Build a CharacterManager or Repository
2 participants