Skip to content

Refactor AttemptRepository to use LinkedHashMap for unique AttemptItems #911

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor AttemptRepository to use LinkedHashMap for unique AttemptItems #911

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions exam/src/main/java/in/testpress/exam/repository/AttemptRepository.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ class AttemptRepository(val context: Context) {
lateinit var attempt : Attempt
private val isOfflineExam: Boolean get() = exam?.isOfflineExam ?: false
var page = 1
val attemptItems = mutableListOf<AttemptItem>()
private val attemptItemMap = linkedMapOf<Int, AttemptItem>()
val attemptItems: List<AttemptItem> get() = attemptItemMap.values.toList()
Comment on lines +34 to +35
Copy link

Choose a reason for hiding this comment

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

medium

This getter creates a new list via toList() every time attemptItems is accessed. This can lead to performance issues if the property is accessed frequently, as each access has a time complexity of O(N) to create the new list, where N is the number of items.

Throughout AttemptRepository, this property is accessed multiple times in different methods, which will result in repeated list creation.

To optimize this, you could cache the list and only regenerate it when attemptItemMap is modified. Here's a conceptual example:

// In class body
private val attemptItemMap = linkedMapOf<Int, AttemptItem>()
private var _cachedAttemptItems: List<AttemptItem> = emptyList()
val attemptItems: List<AttemptItem> get() = _cachedAttemptItems

// In addToAttemptItems()
items.forEach { item -> item.id?.let { attemptItemMap[it] = item } }
_cachedAttemptItems = attemptItemMap.values.toList()

// In clearAttemptItems()
attemptItemMap.clear()
_cachedAttemptItems = emptyList()

This would require changes in addToAttemptItems and clearAttemptItems but would make access to attemptItems an O(1) operation.

Comment on lines +34 to +35
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider thread safety implications of the LinkedHashMap.

The LinkedHashMap implementation effectively prevents duplicates and maintains insertion order, but it's not thread-safe. Given that this repository uses coroutines and may be accessed from multiple threads, consider using Collections.synchronizedMap() or a ConcurrentHashMap with a custom LinkedHashMap wrapper.

-private val attemptItemMap = linkedMapOf<Int, AttemptItem>()
+private val attemptItemMap = Collections.synchronizedMap(linkedMapOf<Int, AttemptItem>())

Additionally, the toList() call on every access to attemptItems creates a new list each time, which may impact performance for frequent access patterns.

🤖 Prompt for AI Agents 
In exam/src/main/java/in/testpress/exam/repository/AttemptRepository.kt around
lines 34 to 35, the use of LinkedHashMap is not thread-safe and the attemptItems
getter creates a new list on each access, which can hurt performance. To fix
this, replace the linkedMapOf with a thread-safe map such as
Collections.synchronizedMap wrapping a LinkedHashMap or use a ConcurrentHashMap
with a LinkedHashMap wrapper to maintain order. Also, consider caching the list
returned by attemptItems or synchronizing access to avoid creating a new list on
every call, ensuring thread safety and better performance.

private var _totalQuestions = 0
val totalQuestions get() = _totalQuestions

Expand Down Expand Up @@ -75,7 +76,7 @@ class AttemptRepository(val context: Context) {
override fun onSuccess(result: TestpressApiResponse<AttemptItem>) {
if (fetchSinglePageOnly) {
_totalQuestions = result.count
attemptItems.addAll(result.results)
addToAttemptItems(result.results)
_attemptItemsResource.postValue(Resource.success(attemptItems))
if (result.hasMore()) {
page++
Expand All @@ -84,11 +85,11 @@ class AttemptRepository(val context: Context) {
}
if (result.hasMore()) {
_totalQuestions = result.count
attemptItems.addAll(result.results)
addToAttemptItems(result.results)
page++
fetchAttemptItems(questionsUrlFrag, fetchSinglePageOnly)
} else {
attemptItems.addAll(result.results)
addToAttemptItems(result.results)
_attemptItemsResource.postValue(Resource.success(attemptItems))
}
}
Expand All @@ -101,6 +102,13 @@ class AttemptRepository(val context: Context) {
}
}

// Temporary fix: In rare cases, fetchAttemptItems() was repeatedly invoked due to unknown causes,
// leading to duplicate entries in the list. To mitigate this, we're using a LinkedHashMap to ensure
// uniqueness by item ID while preserving insertion order.
private fun addToAttemptItems(items: List<AttemptItem>) {
items.forEach { item -> attemptItemMap[item.id] = item }
Copy link

Choose a reason for hiding this comment

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

critical

There's a potential NullPointerException here. The id property of AttemptItem is an Integer and can be null. However, attemptItemMap is declared as LinkedHashMap<Int, AttemptItem>, which expects a non-nullable Int as a key. If item.id is null, trying to use it as a key will cause an NPE due to auto-unboxing from Integer to Int.

You should handle the case where item.id is null to prevent a crash.

Suggested change
items.forEach { item -> attemptItemMap[item.id] = item }
items.forEach { item -> item.id?.let { attemptItemMap[it] = item } }

}

private fun createOfflineAttemptItemItem() {
CoroutineScope(Dispatchers.IO).launch {
if (isAttemptItemsAlreadyCreated()) {
Expand Down Expand Up @@ -369,7 +377,7 @@ class AttemptRepository(val context: Context) {
}

fun clearAttemptItems() {
attemptItems.clear()
attemptItemMap.clear()
}

fun resetPageCount() {
Expand Down