Feature [RM61] Created and Implemented CharactersViewModel #65
Conversation
Built the CharactersViewModel which is injected into the CharactersView from the ContentView.
| if characterType == .rick { | ||
| characters = ricks | ||
| } else { | ||
| characters = morties | ||
| } |
There was a problem hiding this comment.
The function would be more testable with return value private func fetchCharacters() -> [Character].
Also for this length of function you could use
characters = characterType == .rick ? ricks : morties
| private func getTitle() -> String { | ||
| if characterType == .rick { | ||
| return "Ricks" | ||
| } else { | ||
| return "Morties" | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar here,
return characterType == .rick ? "Ricks" : "Morties"
| extension CharactersViewModel { | ||
| enum CharacterType { | ||
| case rick | ||
| case morty | ||
| } | ||
| } |
There was a problem hiding this comment.
Interesting: this is violating the Open Closed principle :P
You would have to modify the ViewModel if you want to add Summer or Jerry
Why don't you define a struct instead?
struct CharacterViewState {
let title: String
let name: String
let description: String
let imageName: String
let imagePosition: CharacterImagePosition
}
If you feed your ViewModel with an array of this model, you won't need to take decisions anymore inside your ViewModel ;)
There was a problem hiding this comment.
I created a CharacterViewState and a CharactersViewState which holds an array of CharacterViewState's (naming could probably be better).
A CharactersViewState is made for each CharacterType in the new ContentViewModel. I don't think there is a need for a CharactersViewModel anymore. Check out my new commit, it should make more sense.
| fetchCharacters() | ||
| } | ||
|
|
||
| private func fetchCharacters() { |
There was a problem hiding this comment.
This ViewModel has 2 responsibilities:
- Feed the views with ViewStates
- Decide how to fetch the data
Can you imagine how you can move one of this responsibility out?
There was a problem hiding this comment.
Moved this out into a CharactersManager class. The class needs to be modified if new CharacterTypes are added, have not been able to find a way around this.
ContentViewModel is now used instead of CharactersViewModel. The ContentViewModel produces CharactersViewStates. The ContentViewModel follows the open-closed principal, however the CharactersManager, CharacterViewStateFactory and CharactersViewStateFactory do not. All these classes and struct will need to be refactored into different files, as of now the ViewState structs, factory classes and CharactersManager are all in the ContentViewModel file.
Moved factories into a dedicated folder and moved CharactersManager into misc folder.
| /* | ||
| struct CharactersView_Previews: PreviewProvider { | ||
| static var previews: some View { | ||
| CharactersView(characters: ricks, title: "Ricks") | ||
| //CharactersView(viewModel: CharactersViewModel(characterType: .rick)) | ||
| //CharactersView(viewModel: CharactersViewModel(characterType: .morty)) | ||
| } | ||
| } | ||
| */ |
There was a problem hiding this comment.
This should be removed. there is no need for commented out code. You have a repo to check for old versions if you really need to :)
https://github.com/orgs/novoda/projects/1#card-65878596
The CharactersView now consumes a CharactersViewModel which is injected into the view from the ContentView. CharactersViewModel provides all the data for the CharactersView.