Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

List of problems! #85

Open
hadilq opened this issue Mar 15, 2022 · 6 comments
Open

List of problems! #85

hadilq opened this issue Mar 15, 2022 · 6 comments

Comments

@hadilq
Copy link
Collaborator

hadilq commented Mar 15, 2022

  • I would suggest avoid using the name of project on classes.

  • Singleton CoroutineScope is a wrong pattern because every async operation needs to have a scope to define when they will be canceled. Check here

  • As we chatted, the interfaces in the data module should moved out to a new module, let's name it data-api, so other modules that needs it could inject the interfaces. Also as I noticed the interfaces of implementation in data module actually are on wrong modules! For instance, the implementation of ArticleDao interface will be generated in the data module so this interface must be beside other Dao interfaces in the so called data-api module.

  • It's hard to understand why you have core module and also common-ui and features/authentication/common-ui modules!

Hope it helps.

@kasem-sm
Copy link
Owner

Hey @hadilq thanks for reviewing the code. I was just following this guide available here for singleton coroutine scope: https://manuelvivo.dev/coroutinescope-hilt

image

@kasem-sm
Copy link
Owner

So the common-ui that is on the project level contains UI components that is required to the whole app. The Composables in it might be required in ui-home as well as in ui-explore.

The other common-ui module which is located inside features/authentication/common-ui contains UI components that is needed by the specific feature only. Here as this common-ui is inside of authentication feature, the components will be required only by ui-auth (login and register screen)

@hadilq
Copy link
Collaborator Author

hadilq commented Mar 16, 2022

As I mentioned, because Google has a document for something doesn't mean it's correct! It just means it has a demand to be documented/standardized.

Regarding common-ui in a specific feature directory, It just means nothing! common means there's a code that is shared among other models, which is not the case here.

@kasem-sm
Copy link
Owner

core module has codes that are needed by almost all modules. The other module is just not common, it's common-ui. Do you mean that I should remove all the common-ui modules in feature directories and merge them with the root common-ui?

@esafirm
Copy link
Contributor

esafirm commented Apr 28, 2022

@kasem-sm I also don't understand why a UI module like ui-auth is outside feature directory?

@kasem-sm
Copy link
Owner

kasem-sm commented May 2, 2022

The thing is UI modules are the ones using the features. Some screen such as the ui-home or ui-explore are using multiple features. As of now, only ui-auth uses a single feature, that is the authentication feature. If I added the ui-auth module inside of features\authentication package, it may cause confusion to some people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants