Sample FruitsMerge #12
Conversation
DzmitryKharko
requested review from
AlexMerzlikin,
eldarmguseynov and
perepechenko
| { | ||
| await UniTask.Yield(); | ||
| if (cancellationToken.IsCancellationRequested) | ||
| { |
There was a problem hiding this comment.
Consider throwing an operation cancelled exception simply passing CT into yield
| /// RootLifetimeScope sets up the DI container for the game application, configuring essential components such as | ||
| /// the entry point (Bootstrap), controllers, and services. This setup ensures efficient dependency management and lifecycle handling during gameplay. | ||
| /// </summary> | ||
| public class RootLifetimeScope : LifetimeScope |
There was a problem hiding this comment.
Consider not to use LifetimeScope cause it's tied to unity engine lifecycle. Try to keep control code centric.
There was a problem hiding this comment.
| protected override async UniTask OnFlowAsync(CancellationToken cancellationToken) | ||
| { | ||
| _view = await _factory.CreateAsync(_data.ResourceId, cancellationToken); | ||
| cancellationToken.ThrowIfCancellationRequested(); |
There was a problem hiding this comment.
you can safely remove all cancellationToken.ThrowIfCancellationRequested(), cause asynchronous operations you're using will throw an exception anyway
| /// Upon creation, each board is positioned randomly within a specified range and visually represented using asset | ||
| /// data obtained from FruitVisualisationProvider. | ||
| /// </summary> | ||
| public class FruitBoardFactory |
There was a problem hiding this comment.
Seems we need IFruitBoardFactory interface to follow Dependency inversion principle
| { | ||
| private const string ShowAnimationName = "AfterLevelScreen_Show"; | ||
|
|
||
| public Button RestartButton => _restartButton; |
There was a problem hiding this comment.
Controllers will be cleaner if we won't expose buttons and other ui components. We can expose button clicked event instead of button
| /// <summary> | ||
| /// AfterLevelScreenView is a MonoBehaviour, manages the view component displayed after completing a game level. | ||
| /// </summary> | ||
| public class AfterLevelScreenView : MonoBehaviour |
There was a problem hiding this comment.
Do we need interfaces for views? If we need to test controllers, then yes
|
|
||
| protected override async UniTask OnFlowAsync(CancellationToken cancellationToken) | ||
| { | ||
| var prefab = await _resourcesProvider.LoadAsync<AfterLevelScreenView>("AfterLevelScreen", cancellationToken); |
There was a problem hiding this comment.
Should we move the view creation flow to some factory?
|
|
||
| private async UniTask FlowAsync() | ||
| { | ||
| while (!CancellationToken.IsCancellationRequested) |
There was a problem hiding this comment.
Better to pass a cancellation token as a parameter
|
|
||
| public async UniTask InitializeAsync(CancellationToken cancellationToken) | ||
| { | ||
| GameConfiguration = await _resourcesProvider.LoadAsync<GameConfiguration>("GameConfiguration", cancellationToken); |
There was a problem hiding this comment.
Is the model a domain model? Seems it should not access _resourcesProvider. It is better to load the game configuration outside of the model (Controller) and pass it as a parameter
| private readonly ResourcesProvider _resourcesProvider; | ||
| private readonly IGameEventsRequestsModel _eventRequestsModel; | ||
|
|
||
| private FruitData[] _fruitsCollected; |
There was a problem hiding this comment.
Minor. _fruitsCollected is a good name for event. For field _collectedFruits will be better
WHAT Create sample for ControllersTree package