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

Improve Readability and Maintainability of Data Purge Function (purge()) #2528

Open
itstanany opened this issue Apr 28, 2024 · 1 comment · May be fixed by #2529
Open

Improve Readability and Maintainability of Data Purge Function (purge()) #2528

itstanany opened this issue Apr 28, 2024 · 1 comment · May be fixed by #2529
Labels
effort:small Small effort - 2 days P3 Low priority issue

Comments

@itstanany
Copy link

itstanany commented Apr 28, 2024

Describe the Issue

This issue proposes a refactoring of the purge function within the engine module for improved code readability and maintainability. The current implementation checks for resource presence, local changes, and force purge flag within nested conditional statements, potentially leading to less readable and maintainable code.

Suggested Changes:

The proposed refactor aims to achieve the same functionality with improved clarity by:

  • Combined check for local changes and force purge: Combine the checks for local changes (localChanges.isNotEmpty()) and the forcePurge flag within a single condition.
  • Concise comments: Add concise comments to explain the logic behind each step.
  • Reduced redundancy:

Benefits:

  • Improved code readability by separating concerns and using clear logic flow.
  • Enhanced maintainability by simplifying conditional checks and reducing nesting.
  • Potentially clearer code for future developers working on this functionality.

Attached Code Snippets:

Include the current and proposed code snippets within the issue description for easy reference:

Current Code:

override suspend fun purge(type: ResourceType, ids: Set<String>, forcePurge: Boolean) {
    db.withTransaction {
      ids.forEach { id ->
        // To check resource is present in DB else throw ResourceNotFoundException()
        selectEntity(type, id)
        val localChangeEntityList = localChangeDao.getLocalChanges(type, id)
        // If local change is not available simply delete resource
        if (localChangeEntityList.isEmpty()) {
          resourceDao.deleteResource(resourceId = id, resourceType = type)
        } else {
          // local change is available with FORCE_PURGE the delete resource and discard changes from
          // localChangeEntity table
          if (forcePurge) {
            resourceDao.deleteResource(resourceId = id, resourceType = type)
            localChangeDao.discardLocalChanges(
              token = LocalChangeToken(localChangeEntityList.map { it.id }),
            )
          } else {
            // local change is available but FORCE_PURGE = false then throw exception
            throw IllegalStateException(
              "Resource with type $type and id $id has local changes, either sync with server or FORCE_PURGE required",
            )
          }
        }
      }
    }
}

Proposed Code:

  override suspend fun purge(type: ResourceType, ids: Set<String>, forcePurge: Boolean) {
    db.withTransaction {
      ids.forEach { id ->
        // 1. Verify resource presence:
        selectEntity(type, id)

        // 2. Check for local changes and handle accordingly:
        val localChanges = localChangeDao.getLocalChanges(type, id)
        if (localChanges.isNotEmpty() && !forcePurge) {
          throw IllegalStateException(
            "Resource with type $type and id $id has local changes, either sync with server or FORCE_PURGE required"
          )
        }

        // 3. Delete resource and discard local changes (if applicable):
        resourceDao.deleteResource(id, type)
        if (localChanges.isNotEmpty()) {
          localChangeDao.discardLocalChanges(id, type)
        }
      }
    }
  }

This approach provides a clear and concise issue description for the Android FHIR repository, promoting collaboration and potential code improvement.

Would you like to work on the issue?
Yes 🚀

itstanany added a commit to itstanany/android-fhir that referenced this issue Apr 28, 2024
itstanany added a commit to itstanany/android-fhir that referenced this issue Apr 28, 2024
@MJ1998
Copy link
Collaborator

MJ1998 commented May 1, 2024

Thanks @itstanany. This looks good to me!

itstanany added a commit to itstanany/android-fhir that referenced this issue May 12, 2024
@aditya-07 aditya-07 added P3 Low priority issue effort:small Small effort - 2 days labels May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort:small Small effort - 2 days P3 Low priority issue
Projects
Status: New
3 participants