Command #21
Command #21
Conversation
|
|
||
| ```swift | ||
| protocol ImageCommand { | ||
| class CarouselViewModel { | ||
| private let images: [String] |
There was a problem hiding this comment.
Why would images be private on a view model?
And if images is an array of String, should this be called imagePaths?
| private(set) var currentIndex: Int | ||
|
|
||
| init(images: [String]) { | ||
| self.currentIndex = 0 |
There was a problem hiding this comment.
Why set currentIndex to 0 in the init when you can set a default value inline?
| func navigateToPreviousItem() { | ||
| currentIndex = (currentIndex - 1) % images.count | ||
| } |
There was a problem hiding this comment.
This code will crash the app if the index is currently at 0. Read up on issues with using Remainder operator with negative numbers.
| func navigateToItem(at index: Int) { | ||
| currentIndex = index | ||
| } |
There was a problem hiding this comment.
This code will crash the app if an invalid index is passed in.
| init(carouselViewModel: CarouselViewModel) { | ||
| self.carouselViewModel = carouselViewModel | ||
|
|
||
| self.tapInvoker = TapInvoker() |
There was a problem hiding this comment.
You can't init TapInvoker this way. Try.
Tap Invoker requires parameters. Same goes for all the other Invokers you init in this class other than TimerInvoker (but that is only because TimerInvoker is set up incorrectly).
| // Output: | ||
| // Carousel starting index: 0 | ||
| // Carousel index: 1 | ||
| // Carousel index: 0 | ||
| // Carousel index: 1 | ||
| // Carousel index: 2 | ||
| // Carousel index: 1 |
There was a problem hiding this comment.
Suggest removing this output, in line with removing the print statements.
| // Output: | ||
| // Carousel index updated via timer: 2 | ||
| // Carousel index updated via timer: 3 | ||
| // Carousel index updated via timer: 0 | ||
| // Carousel index updated via timer: 1 | ||
| // Carousel index updated via timer: 2 | ||
| // Carousel index updated via timer: 3 | ||
| // Carousel index updated via timer: 0 | ||
| // ... |
| func nextImage() { | ||
| currentIndex = (currentIndex + 1) % images.count | ||
| init(carouselViewModel: CarouselViewModel) { | ||
| self.carouselViewModel = carouselViewModel |
There was a problem hiding this comment.
Why are you assigning the view model to a property on the class if it's only used in the init?
If you pass in the commands already configured, then the view needn't have knowledge of the receiver (the view model). See comment below about you configuring all the commands in the init, instead of passing them to the view already configured.
|
|
||
| func simulateTapForNextButton() { | ||
| tapInvoker.nextButtonTapped() | ||
| print("Carousel index: " + "\(viewModel.currentIndex)") |
There was a problem hiding this comment.
There is NO property on your class called viewModel. So how is this print statement even going to work.
| self.tapInvoker = TapInvoker() | ||
| self.tapInvoker.navigateToNextCommand = NavigateToNextItemCommand(receiver: carouselViewModel) | ||
| self.tapInvoker.navigateToPreviousCommand = NavigateToPreviousItemCommand(receiver: carouselViewModel) | ||
| self.tapInvoker.navigateToItemCommand = NavigateToNextItemCommand(receiver: carouselViewModel) |
There was a problem hiding this comment.
You pass a NavigateToNextItemCommand to tapInvoker.navigateToItemCommand, when you should be passing a NavigateToItemCommand. But doing so doesn't make sense since NavigateToItemCommand takes an Index, and you don't have the index yet!
| var images: [String] | ||
| var currentIndex: Int | ||
| class CarouselView { | ||
| let viewModel: CarouselViewModel |
There was a problem hiding this comment.
Which class needs access to this view model from outside this class?
Also, why set the view model property on the class if it's only needed in the init?
Lastly, by injecting the view model into the view you are essentially injecting the receiver into the invoker, coupling them. The whole point of the Command pattern is to decouple them. In order for the view and view model to be decoupled, the view needs to have zero knowledge of the view mode. You can achieve this my injecting invokers into the view, already initid with their receiver (the view model).
| struct TapInvoker { | ||
| let receiver: CarouselViewModel | ||
|
|
||
| func rightArrowButtonTapped() { | ||
| let navigateToNextCommand = NavigateToNextItemCommand(receiver: receiver) | ||
| navigateToNextCommand.execute() | ||
| } | ||
|
|
||
| func leftArrowButtonTapped() { | ||
| let navigateToPreviousCommand = NavigateToPreviousItemCommand(receiver: receiver) | ||
| navigateToPreviousCommand.execute() | ||
| } | ||
|
|
||
| func buttonTappedForIndex(_ index: Int) { | ||
| let navigateToItemCommand = NavigateToItemCommand(receiver: receiver, index: index) | ||
| navigateToItemCommand.execute() | ||
| } | ||
| } |
There was a problem hiding this comment.
All your invokers have the same issue as your view: they are coupled to the receiver. You can fix this by injecting the relevant commands, already inited with a receiver, into your invokers.
No description provided.