Feature/Show groups list #45
Conversation
There was a problem hiding this comment.
Cara, muito bom o PR.
Me surpeendi com a tua evolução. Acho que tu entendeu bem a ideia do asyncState e do projeto num geral.
Deixei alguns comentários aí, vamos falando sobre eles caso tu tenha alguma dúvida. Deixa que eu marco as conversas como resolved conforme vc for fazendo as alterações
| if (user.authType == Anonymous) { | ||
| throw NotAuthorizedForItException() | ||
| } | ||
| // if (user.authType == Anonymous) { |
|
|
||
| //Faz sentido essa Exception que eu criei? | ||
| class GetGroupsListException( |
There was a problem hiding this comment.
faz sim. se essa request pro firebase falhar em produção vai ser bem fácil ver ela no crashlytics.
A única coisa que vc nao precisava enviar é o userId no logo, pq eu fiz uma magia pra sempre colocar o userId em todas as exceptions que logamos.
| @@ -66,16 +66,21 @@ sealed class AppRouteDestination(val route: String) { | |||
| @Composable | |||
| fun HomeNavGraph(homeUiState: HomeUiState, subscription: Subscription) { | |||
|
|
|||
| val groupsViewModel = getStateViewModel<GroupTabViewModel> { | |||
| parametersOf() | |||
There was a problem hiding this comment.
Não sei também, vou remover ele aqui
| override fun fromUi(uiEvent: GroupsUIEvent) { | ||
| ) : StateViewModel<CreateGroupData, CreateGroupUIEvent>(CreateGroupData(), loading = false) { | ||
|
|
||
| override fun fromUi(uiEvent: CreateGroupUIEvent) { |
There was a problem hiding this comment.
pequeno detalhe: em kotlin se usa camel case por convenção, mesmo para abreviações. Então preferencialmente vc pode trocar o UI por Ui nessa classe
| private val repository: GroupRepository, | ||
| private val userRepository: UserRepository |
There was a problem hiding this comment.
lá vem textão:
na medida em que as regras de negócio na tela de grupo forem crescendo, talvez mais repositórios sejam necessários.
não é uma boa implementar regras de negócio no ViewModel, pq ele é uma classe que serve pra outras finalidades, como gerenciar o estado da UI.
Portanto, talvez seja melhor criar uma class UseCase, tipo GetGroupsUseCase, e fazer com que esse usecase use os repositórios pra implementar as regras de negócio.
A vantagem disso é que tiramos as regras de negócio de uma classe que interage com uma biblioteca externa, que é caso do ViewModel.
é um detalhe besta em um primeiro momento, mas essas pequenas decisões tem um peso grande conforme o projeto vai crescendo.
There was a problem hiding this comment.
Pode crer, faz sentido. Como essa tela não ia ter useCase por enquanto acabei fazendo tudo no viewModel mesmo, mas vou me atentar mais nisso nas próximas.
| InfoTemplate( | ||
| icon = { Icon(imageVector = Icons.Default.Person, contentDescription = "groups tab") }, | ||
| title = { Text("Sem grupos") }, |
There was a problem hiding this comment.
eu acho uma boa já colocar esses textos nos strings resources, porque assim evitamos que por algum vacilo a gente esqueça de remover um deles
| getGroups() | ||
| } | ||
|
|
||
| fun getGroups() { |
There was a problem hiding this comment.
tem algum motivo pra essa função ser pública?
There was a problem hiding this comment.
Sim, ele é usado no GroupTabScreen, pra quando eu adicionar um novo grupo a tela ja mostrar a lista atualizada, ai chama essa função de novo.
| @@ -61,9 +62,20 @@ class GroupCollection( | |||
| batch.commit().awaitWithTimeout() | |||
| } | |||
|
|
|||
| suspend fun getGroupsByUserId(founder: User) : List<GroupEntry> { | |||
There was a problem hiding this comment.
alerta de dica irrelevante:
como esse já é o GroupCollection, acho que da pra tirar o Groups do nome desse método, assim evita duplicação de palavras
|
Kudos, SonarCloud Quality Gate passed!
|
The problem
We need to show the created groups in groupTabScreen
The solution
Just listing all groups from user, without details for now. This task implements the list and updates in case of a new group was added.