refactor(core): migrate from dbflow to room for client

[biplab1]

Fixes - Jira-#341

Didn't create a Jira ticket, click here to create new.

Please Add Screenshots If there are any UI changes.

Before	After

Please make sure these boxes are checked before submitting your pull request - thanks!

• Run the static analysis check ./gradlew check or ci-prepush.sh to make sure you didn't break anything

• If you have multiple commits please combine them into one commit by squashing them.

[HekmatullahAmin]

Use emptyList() (or mutableListOf() if mutability is necessary) in instead of ArrayList(). It is more efficient to use emptyList() since it prevents needless object construction and offers superior immutability by default. For improved efficiency and clarity, use emptySet(), emptyMap(), listOf(), setOf(), and mapOf() when appropriate.

[biplab1]

I will do as suggested.

[biplab1]

Suggested changes implemented

[biplab1]

Use emptyList() (or mutableListOf() if mutability is necessary) in instead of ArrayList(). It is more efficient to use emptyList() since it prevents needless object construction and offers superior immutability by default. For improved efficiency and clarity, use emptySet(), emptyMap(), listOf(), setOf(), and mapOf() when appropriate.

@HekmatullahAmin Can you cite some resources that support these statements?

[HekmatullahAmin]

Sorry @biplab1, I didn't notice your questions earlier.

Regarding sources, there are plenty of resources you can read about this, including the official Kotlin documentation. Using emptyList() is better than ArrayList() because it provides immutability by default, which prevents accidental modifications to the list.

For example:

class IdentifierTemplate(
    val allowedDocumentTypes: List<DocumentType>? = ArrayList() // Mutable list
)

In this case, even though allowedDocumentTypes is declared as val (immutable reference), the list itself is mutable because ArrayList() is used. This means the contents of the list can still be modified:

val template = IdentifierTemplate()
template.allowedDocumentTypes?.add(DocumentType()) // This works, but mutability might not be intended

To prevent accidental modifications, you can use emptyList() instead:

class IdentifierTemplate(
    val allowedDocumentTypes: List<DocumentType>? = emptyList() // Immutable list
)

Now, the list cannot be modified:

val template = IdentifierTemplate()
// template.allowedDocumentTypes?.add(DocumentType()) // This would cause a compile-time error

By using emptyList(), you ensure that the list cannot be changed accidentally, making your code safer and more predictable. If mutability is needed, you can use mutableListOf() instead.

[niyajali]

Here's few things to change, these changes are not nessacary as per your task, but as you modifying repository classes this changes should required

1. Non-suspend functions returning Flow:
   If a function returns a Flow, it should not be marked as suspend because Flow is already designed to handle asynchronous streams.

   fun getItems(): Flow<List<Item>> {
       // Return a Flow of List<Item>
   }

2. Convert List<T> to Flow<List<T>>:
   If a function currently returns a List<T>, it should be modified to return a Flow<List<T>> to align with reactive programming practices.

   fun getItems(): Flow<List<Item>> {
       // Convert List<Item> to Flow<List<Item>>
   }

3. Suspend functions for actions (create/update/delete):
   If a function performs an action like creating, updating, or deleting, it should be marked as suspend and return a Result or Unit.

   suspend fun createItem(item: Item): Result<Unit> {
       // Perform create action and return Result
   }
   
   suspend fun updateItem(item: Item): Result<Unit> {
       // Perform update action and return Result
   }
   
   suspend fun deleteItem(itemId: String): Result<Unit> {
       // Perform delete action and return Result
   }

[biplab1]

@niyajali Thanks for reviewing! I will do as suggested.

[itsPronay]

Remove Flow from here. Don't use Flow in dao unless we are retrieving list. Check the entire DAO

[itsPronay]

@biplab1 my apologies. Ignore the previous comment.
Check if we need a single item. If yes add 'LIMIT 1`

[biplab1]

In DatabaseHelperClient.kt, this function was returning Observable<GroupWithAssociations>, so it was changed to Flow<GroupWithAssociations>

[itsPronay]

@biplab1 This is the function right?
it should be this instead

@Query("SELECT * FROM Client WHERE groupId = :groupId")
    fun getGroupAssociateClients(groupId: Int): Flow<List<Client>>

[biplab1]

The original function returned GroupWithAssociations, the one you suggest returns List<Client>. GroupWithAssociations has a List<Client> in it but also some other things.

[itsPronay]

Save, delete and update should be a suspend function always

[biplab1]

Does your statement hold true even when the function returns a flow?

[itsPronay]

Yes,
save, delete and update should not return flow because they are one time operations

[biplab1]

It will be implemented.

[biplab1]

Suggested changes implemented.

[itsPronay]

emptyMap() would be better

[biplab1]

It will be implemented.

[biplab1]

Suggested changes implemented.

[itsPronay]

mBaseApiManager is used for the api call.

[biplab1]

I do not understand your statement, could you please provide more details?

[itsPronay]

This is for the api call. But we are migrating the database only right?

But if you want to migrate it, we need to change it to something like that.

@DELETE(APIEndPoint.CLIENTS + "/{clientId}/images")
    suspend fun deleteClientImage(@Path("clientId") clientId: Int): ResponseBody

 fun deleteClientImage(clientId: Int): Flow<ResponseBody> {
        return flow { emit( mBaseApiManager.clientsApi.deleteClientImage(clientId) )}
    }

[biplab1]

I was following this PR #2292, where DataManagerLoan.kt has been migrated.

[biplab1]

@itsPronay Thanks for reviewing! I will look into your suggestions.

[biplab1]

Here's few things to change, these changes are not nessacary as per your task, but as you modifying repository classes this changes should required

1. **Non-suspend functions returning `Flow`**:
   If a function returns a `Flow`, it should not be marked as `suspend` because `Flow` is already designed to handle asynchronous streams.
   ```kotlin
   fun getItems(): Flow<List<Item>> {
       // Return a Flow of List<Item>
   }
   ```

2. **Convert `List<T>` to `Flow<List<T>>`**:
   If a function currently returns a `List<T>`, it should be modified to return a `Flow<List<T>>` to align with reactive programming practices.
   ```kotlin
   fun getItems(): Flow<List<Item>> {
       // Convert List<Item> to Flow<List<Item>>
   }
   ```

3. **Suspend functions for actions (create/update/delete)**:
   If a function performs an action like creating, updating, or deleting, it should be marked as `suspend` and return a `Result` or `Unit`.
   ```kotlin
   suspend fun createItem(item: Item): Result<Unit> {
       // Perform create action and return Result
   }
   
   suspend fun updateItem(item: Item): Result<Unit> {
       // Perform update action and return Result
   }
   
   suspend fun deleteItem(itemId: String): Result<Unit> {
       // Perform delete action and return Result
   }
   ```

@niyajali Can you cite some resources that support these statements?