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

v055o #1581

Open
wants to merge 212 commits into
base: main
Choose a base branch
from
Open

v055o #1581

wants to merge 212 commits into from

Conversation

hussainmohd-a
Copy link
Collaborator

No description provided.

Copy link

@soshial soshial Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for meddling into your codebase, but this is a completely wrong approach in Android. It can lead to lifecycle issues, memory leaks, and difficulty in managing coroutine scopes. Adapter and ViewHolder classes should be lightweight and contain almost no logic or calculations, especially the coroutines. It should only passively receive data from Fragment/ViewModel, sometimes map it and draw it. Please, perform calculations and launch coroutines in a ViewModel or a similar layer and pass the processed data to the adapter. This keeps the adapter lean and focused on UI concerns, adhering to the Single Responsibility Principle.

My bugs that I experienced is probably because of this. ViewHolder might live 1-5 seconds and be destroyed, hence must not launch any coroutines. @hussainmohd-a @ignoramous

Copy link
Collaborator Author

@hussainmohd-a hussainmohd-a Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soshial: Thank you for your valuable feedback and for pointing out the potential issues with our current approach.
Our current implementation uses coroutines in the adapter due to the specific nature of our data fetching, which involves retrieving data from both the database and a network library (Go lib). This approach is indeed a temporary solution to meet our immediate needs. We acknowledge that this is not the ideal or long-term strategy and may lead to the issues you've mentioned.

We are actively working on optimizing this setup and plan to refactor it to ensure that data processing and coroutine management are handled in a more appropriate layer, such as the ViewModel.

Your insights are greatly appreciated and will help us improve our codebase.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry for meddling into your codebase

No, thank you.

I encourage you to send pull requests, if you have the time :)

Previously, we had @HrBDev and @Ch4t4r help out immensely with the codebase.

while (input.read(buffer).also { bytesRead = it } != -1) {
output.write(buffer, 0, bytesRead)
}
output.close()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close in finally?

Also, is there a need for copy to close streams when the callers are using copy within kt's use{} block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been removed: 4083e75

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

Successfully merging this pull request may close these issues.

3 participants