SlimeKT icon indicating copy to clipboard operation
SlimeKT copied to clipboard

List of problems!

Open hadilq opened this issue 2 years ago • 6 comments

  • 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.

hadilq avatar Mar 15 '22 22:03 hadilq

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 avatar Mar 16 '22 07:03 kasem-sm

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)

kasem-sm avatar Mar 16 '22 07:03 kasem-sm

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.

hadilq avatar Mar 16 '22 12:03 hadilq

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?

kasem-sm avatar Mar 16 '22 13:03 kasem-sm

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

esafirm avatar Apr 28 '22 01:04 esafirm

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.

kasem-sm avatar May 02 '22 00:05 kasem-sm