I dont think there is any need to shorten the variable name here or anywhere else like this

There's a possibility that we are swallowing an error in the case that the error is not an NSError so we should add an else to catch these with a more generic error response. Something like Error: wtf happened here. We don't know. Probably couldnt parse the error as NSError though

excellent erro message 😂

May be worth extracting this all into a neat little error handling function. Pyramids of doom are appearing, normally a good sign

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

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

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

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()

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

@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?

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 😂

Same comment about logic here. You may need a tiny VM for this View

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