Feature [RM88] Handle Server Error #91
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import Foundation | ||
|
|
||
| protocol RickAndMortyServiceProtocol { | ||
| func fetchData<T:Decodable>(url: URL, success: @escaping (T) -> (), error: @escaping (NSError?) -> ()) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,15 @@ final class CharacterListViewModel: ObservableObject { | |
|
|
||
| struct CharacterListViewState { | ||
| let title: String = "Characters" | ||
| var errorMessage: String? = nil | ||
| var characters: [Character] | ||
| var state: State = .doneLoading | ||
|
|
||
| enum State { | ||
| case loading | ||
| case doneLoading | ||
| case error | ||
| } | ||
| } | ||
|
|
||
| private let characterRepository: CharacterRepositoryProtocol = CharacterRepository() | ||
|
|
@@ -16,11 +24,28 @@ final class CharacterListViewModel: ObservableObject { | |
| } | ||
|
|
||
| func loadCharacters() { | ||
| characterListViewState.state = .loading | ||
|
|
||
| characterRepository.getCharacters { characters in | ||
| self.characterListViewState.state = .doneLoading | ||
| self.characterListViewState.errorMessage = nil | ||
|
|
||
| for character in characters { | ||
| self.characterListViewState.characters.append(character) | ||
| } | ||
| } error: { error in | ||
|
|
||
| self.characterListViewState.state = .error | ||
|
|
||
| if let err = error { | ||
| if err.code == 4864 { | ||
| self.characterListViewState.errorMessage = "Wubba Lubba Dub Dub! There was a problem loading the characters." | ||
|
|
||
| } else { | ||
| self.characterListViewState.errorMessage = err.localizedDescription | ||
| } | ||
|
|
||
| self.characterListViewState.errorMessage?.append(" Tap the refresh button to try again..") | ||
| } | ||
|
There was a problem hiding this comment. May be worth extracting this all into a neat little error handling function. Pyramids of doom are appearing, normally a good sign There was a problem hiding this comment. yeah definitely, for simplicity Im just doing a custom message for that code at the moment. All the rest of the errors just display the localized description |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,12 +5,21 @@ struct CharacterListView: View { | |||||||||||||
|
|
||||||||||||||
| var body: some View { | ||||||||||||||
| NavigationView { | ||||||||||||||
| ZStack { | ||||||||||||||
| List(viewModel.characterListViewState.characters, id: \.id) { character in | ||||||||||||||
| CharacterCard(viewModel: CharacterCardViewModel(character: character)) | ||||||||||||||
| .onAppear(perform: { | ||||||||||||||
| viewModel.loadIfNeeded(characterID: character.id) | ||||||||||||||
| }) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if viewModel.characterListViewState.state == .error { | ||||||||||||||
| ErrorView(errorMessage: viewModel.characterListViewState.errorMessage, refreshAction: viewModel.loadCharacters) | ||||||||||||||
| } else if viewModel.characterListViewState.state == .loading { | ||||||||||||||
| ProgressView() | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+16
to
+20
There was a problem hiding this comment. this logic should be in the VM. It shouldnt be possible to unit test the view :) There was a problem hiding this comment. Whats your suggestion to fix this? I think in swiftUI its normal to use conditionals based on the state in the view There was a problem hiding this comment. So what happens in most cases now is you would have a reactive chain connected to the state, so whenever that changes the view would update to
Suggested change
In the viewModel: This isnt a perfect solution, as ideally the ViewModel doesnt know anything about views either and this function would exist inside a ViewFactory of some sort that you would pass as a dependency to the VM. That would look something like: Now the ViewModel would be updated with the init to look like so: then your viewForState func would be even cleaner and look like this: There was a problem hiding this comment. @wrumble Do you think There was a problem hiding this comment. Yea, i guess so, whatever your preference i guess, i just made the step from moving it to view model then to factory didnt think any further 😂 |
||||||||||||||
| } | ||||||||||||||
| .listStyle(PlainListStyle()) | ||||||||||||||
| .navigationTitle(viewModel.characterListViewState.title) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| // | ||
| // ErrorView.swift | ||
| // Rick And Morty | ||
| // | ||
| // Created by Scottie Gray on 2021-08-24. | ||
| // Copyright © 2021 Novoda. All rights reserved. | ||
| // | ||
|
|
||
| import SwiftUI | ||
|
|
||
| struct ErrorView: View { | ||
| let errorMessage: String? | ||
| let refreshAction: () -> () | ||
|
|
||
| var body: some View { | ||
| if let error = errorMessage { | ||
|
There was a problem hiding this comment. Same comment about logic here. You may need a tiny VM for this View |
||
| VStack { | ||
| Text(error) | ||
| .foregroundColor(.gray) | ||
| .multilineTextAlignment(.center) | ||
| .padding() | ||
| Button(action: refreshAction, label: { | ||
| Image(systemName: "arrow.clockwise") | ||
| .resizable() | ||
| .frame(width: 30, height: 30, alignment: /*@START_MENU_TOKEN@*/.center/*@END_MENU_TOKEN@*/) | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| struct ErrorView_Previews: PreviewProvider { | ||
| static var previews: some View { | ||
| ErrorView(errorMessage: "Wubba Lubba Dub Dub! There was a problem loading. Tap the button to try again.", refreshAction: {}) | ||
|
|
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a possibility that we are swallowing an error in the case that the error is not an
NSErrorso we should add an else to catch these with a more generic error response. Something likeError: wtf happened here. We don't know. Probably couldnt parse the error as NSError though