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.

Sign up for GitHub

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 [RM88] Handle Server Error #91

Merged
merged 2 commits into from
Aug 27, 2021
Merged

Feature [RM88] Handle Server Error #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc5ffd8
Error handling and progress view
swg99 Aug 24, 2021
b99cc75
Adressed comments
swg99 Aug 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions Rick-and-Morty/CharacterRepository.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import Foundation

protocol CharacterRepositoryProtocol {
func getCharacters(completion: @escaping (([Character]) -> Void))
func getCharacters(completion: @escaping (([Character]) -> Void), fail: @escaping ((NSError?) -> Void))
}

final class CharacterRepository: CharacterRepositoryProtocol {
private var characterPageURL: URL? = RickAndMortyService.baseURL.appendingPathComponent("character")

private let rickAndMortyService: RickAndMortyServiceProtocol = RickAndMortyService()

func getCharacters(completion: @escaping (([Character]) -> Void)) {
func getCharacters(completion: @escaping (([Character]) -> Void), fail: @escaping ((NSError?) -> Void)) {
if let url = characterPageURL {
rickAndMortyService.fetchData(url: url) { (charactersResponse: CharacterResponse) in
if let nextURLString = charactersResponse.info.next {
Expand All @@ -21,9 +21,8 @@ final class CharacterRepository: CharacterRepositoryProtocol {
}

completion(charactersResponse.characters)
} error: { error in
print(error.debugDescription)
return
} fail: { (error: NSError?) in
fail(error)
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Rick-and-Morty/EpisodeRepository.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ final class EpisodeRepository: EpisodeRepositoryProtocol {
rickAndMortyService.fetchData(url: url) { (episode: Episode) in
EpisodeRepository.cachedEpisodeNames[urlString] = episode
completion(episode)
} error: { error in
} fail: { error in
print(error.debugDescription)
return
}
Expand Down
4 changes: 4 additions & 0 deletions Rick-and-Morty/Rick And Morty.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
1711B39E26B1898100BE935B /* CharacterListView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1711B39D26B1898100BE935B /* CharacterListView.swift */; };
174655E826CBB74100A52819 /* EpisodeDetailView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 174655E726CBB74100A52819 /* EpisodeDetailView.swift */; };
174655EA26CBBD2300A52819 /* EpisodeDetailViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 174655E926CBBD2300A52819 /* EpisodeDetailViewModel.swift */; };
174878F626D4E3CA004293AB /* ErrorView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 174878F526D4E3CA004293AB /* ErrorView.swift */; };
174B064B26C6611D0080ADD0 /* RickAndMortyService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 174B064A26C6611D0080ADD0 /* RickAndMortyService.swift */; };
174B064D26C661470080ADD0 /* RickAndMortyServiceProtocol.swift in Sources */ = {isa = PBXBuildFile; fileRef = 174B064C26C661470080ADD0 /* RickAndMortyServiceProtocol.swift */; };
174B065126C671580080ADD0 /* CharacterRepository.swift in Sources */ = {isa = PBXBuildFile; fileRef = 174B065026C671580080ADD0 /* CharacterRepository.swift */; };
Expand Down Expand Up @@ -41,6 +42,7 @@
1711B39D26B1898100BE935B /* CharacterListView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CharacterListView.swift; sourceTree = "<group>"; };
174655E726CBB74100A52819 /* EpisodeDetailView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EpisodeDetailView.swift; sourceTree = "<group>"; };
174655E926CBBD2300A52819 /* EpisodeDetailViewModel.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EpisodeDetailViewModel.swift; sourceTree = "<group>"; };
174878F526D4E3CA004293AB /* ErrorView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ErrorView.swift; sourceTree = "<group>"; };
174B064A26C6611D0080ADD0 /* RickAndMortyService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RickAndMortyService.swift; sourceTree = "<group>"; };
174B064C26C661470080ADD0 /* RickAndMortyServiceProtocol.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RickAndMortyServiceProtocol.swift; sourceTree = "<group>"; };
174B065026C671580080ADD0 /* CharacterRepository.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CharacterRepository.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -140,6 +142,7 @@
1711B39D26B1898100BE935B /* CharacterListView.swift */,
17588BAE26C273BB008ECC31 /* CharacterCard.swift */,
174655E726CBB74100A52819 /* EpisodeDetailView.swift */,
174878F526D4E3CA004293AB /* ErrorView.swift */,
174941D126CD1216001AFD9A /* Utilities */,
);
path = Views;
Expand Down Expand Up @@ -308,6 +311,7 @@
B811686D1CFF1C9900301A0A /* AppDelegate.swift in Sources */,
174B065926C680850080ADD0 /* EpisodeRepository.swift in Sources */,
174655EA26CBBD2300A52819 /* EpisodeDetailViewModel.swift in Sources */,
174878F626D4E3CA004293AB /* ErrorView.swift in Sources */,
);
runOnlyForDeploymentPostprocessing = 0;
};
Expand Down
17 changes: 12 additions & 5 deletions Rick-and-Morty/Rick And Morty/Services/RickAndMortyService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ import Foundation
final class RickAndMortyService: RickAndMortyServiceProtocol {
static let baseURL = URL(string: "https://rickandmortyapi.com/api")!

func fetchData<T:Decodable>(url: URL, success: @escaping (T) -> (), error: @escaping (Error?) -> ()) {
func fetchData<T:Decodable>(url: URL, success: @escaping (T) -> (), fail: @escaping (NSError?) -> ()) {

let request = URLRequest(url: url)

URLSession.shared.dataTask(with: request) { data, response, requestError in
Expand All @@ -16,15 +17,21 @@ final class RickAndMortyService: RickAndMortyServiceProtocol {
success(decodedData)
}
} catch let decodingError {
DispatchQueue.main.async {
error(decodingError)
if let error = decodingError as NSError? {
DispatchQueue.main.async {
fail(error)
}
} else {
print("Unknown Error: \(decodingError.localizedDescription)")
}
}
} else {
if requestError != nil {
if let error = requestError as NSError? {
DispatchQueue.main.async {
error(requestError)
fail(error)
}
} else {
print("Unknown Error: \(String(describing: requestError?.localizedDescription))")
}
}
}.resume()
Expand Down
2 changes: 1 addition & 1 deletion Rick-and-Morty/Rick And Morty/Services/RickAndMortyServiceProtocol.swift
View file
Open in desktop
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 (Error?) -> ())
func fetchData<T:Decodable>(url: URL, success: @escaping (T) -> (), fail: @escaping (NSError?) -> ())
}
29 changes: 27 additions & 2 deletions Rick-and-Morty/Rick And Morty/ViewModels/CharacterListViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -16,11 +24,28 @@ final class CharacterListViewModel: ObservableObject {
}

func loadCharacters() {
characterRepository.getCharacters {
characters in
characterListViewState.state = .loading

characterRepository.getCharacters { characters in
self.characterListViewState.state = .doneLoading
self.characterListViewState.errorMessage = nil

for character in characters {
self.characterListViewState.characters.append(character)
}
} fail: { error in

self.characterListViewState.state = .error

if let error = error {
if error.code == 4864 {
self.characterListViewState.errorMessage = "Wubba Lubba Dub Dub! There was a problem loading the characters."
Copy link
Contributor

Choose a reason for hiding this comment

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

excellent erro message 😂

} else {
self.characterListViewState.errorMessage = error.localizedDescription
}
Comment on lines +41 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally would still extract this to a function called handle(error: NSError) as its cleaner and gets this loadCharactes function a bit closer to Singular responsibility :)


self.characterListViewState.errorMessage?.append(" Tap the refresh button to try again..")
}
}
}

Expand Down
19 changes: 14 additions & 5 deletions Rick-and-Morty/Rick And Morty/Views/CharacterListView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,21 @@ struct CharacterListView: View {

var body: some View {
NavigationView {
List(viewModel.characterListViewState.characters, id: \.id) { character in
CharacterCard(viewModel: CharacterCardViewModel(character: character))
.onAppear(perform: {
viewModel.loadIfNeeded(characterID: character.id)
})
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
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic should be in the VM. It shouldnt be possible to unit test the view :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whats your suggestion to fix this? I think in swiftUI its normal to use conditionals based on the state in the view

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ErrorView or ProgressView. In your case though id have a function in the viewModel called viewForState()

Suggested change
if viewModel.characterListViewState.state == .error {
ErrorView(errorMessage: viewModel.characterListViewState.errorMessage, refreshAction: viewModel.loadCharacters)
} else if viewModel.characterListViewState.state == .loading {
ProgressView()
}
viewModel.viewForState()

In the viewModel:

    func viewForState() -> View {
        if viewModel.characterListViewState.state == .error {
            return ErrorView(errorMessage: characterListViewState.errorMessage, refreshAction: loadCharacters)
        } else if characterListViewState.state == .loading {
            return ProgressView()
        }
    }

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:

import SwiftUI

protocol ViewFactoryProtocol {
    func viewForState(errorMessage: String, refreshAction: () -> Void) -> View
}

class ViewFactory {
    static func viewForState(errorMessage: String, refreshAction: () -> Void) -> View {
        if viewModel.characterListViewState.state == .error {
            return ErrorView(errorMessage: characterListViewState.errorMessage, refreshAction: loadCharacters)
        } else if characterListViewState.state == .loading {
            return ProgressView()
        }
    }
}

The factory name `viewForState` could be more descriptive but my morning brain aint working for now.

Hope this is clear! Ping me if not :)

Now the ViewModel would be updated with the init to look like so:

private let viewFactory: ViewFactoryProtocol

init(viewFactory: ViewFactoryProtocol) {
    self.viewFactory = viewFactory
}

then your viewForState func would be even cleaner and look like this:

func viewForState() -> View {
    return viewFactory.viewForState(errorMessage: characterListViewState.errorMessage, refreshAction: loadCharacters)
}

Copy link

Choose a reason for hiding this comment

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

@wrumble Do you think viewForState() should be in viewmodel? Wouldn't that make view model less testable? If this would not be swift UI the logic we're talking about would be in ViewControllers which belong to View layer. If we want to extract that we could call the factory from CharacterListView.swift and test the factory. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

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)
}
}
Expand Down
37 changes: 37 additions & 0 deletions Rick-and-Morty/Rick And Morty/Views/ErrorView.swift
View file
Open in desktop
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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: {})

}
}