Louvorg/ReaderForSelfoss-multiplatform
Louvorg/ReaderForSelfoss-multiplatform
3
2
Fork 2
Code Issues 18 Pull Requests Actions Projects 1 Releases 65 Wiki Activity

network #28

Merged
AmineB merged 28 commits from davidoskky/ReaderForSelfoss-multiplatform:network into master 2022-08-22 19:01:16 +00:00
Conversation 12 Commits 28 Files Changed 14 +142 -77
1 changed files with 142 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 4f32097821 - Show all commits

219
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
View File

@ -48,40 +48,65 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
}
suspend fun getNewerItems(): ArrayList<SelfossModel.Item> {
// TODO: Check connectivity, use the updatedSince parameter
val fetchedItems = api.getItems(displayedItems.type,
settings.getString("prefer_api_items_number", "200").toInt(),
offset = 0,
tagFilter?.tag,
sourceFilter?.id?.toLong(),
searchFilter,
null)
// TODO: Use the updatedSince parameter
if (isConnectionAvailable.value) {
val fetchedItems = api.getItems(
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 18:20:57 +00:00
Outdated
Review
Copy Link

isConnectionAvailable.value && !offlineOverride should be refactored inside a method named isNetworkAvailable()

`isConnectionAvailable.value && !offlineOverride` should be refactored inside a method named `isNetworkAvailable()`
displayedItems.type,
settings.getString("prefer_api_items_number", "200").toInt(),
offset = 0,
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 19:39:16 +00:00
Outdated
Review
Copy Link

There should be a message emited on network available too.

There should be a message emited on network available too.
tagFilter?.tag,
sourceFilter?.id?.toLong(),
searchFilter,
null
)
if (fetchedItems != null) {
items = ArrayList(fetchedItems)
if (fetchedItems != null) {
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 14:12:50 +00:00
Outdated
Review
Copy Link

This block should be after the else, since the db will work as a fallback and we'll get the items from there.

Same for the other block later.

This block should be after the else, since the db will work as a fallback and we'll get the items from there. Same for the other block later.
davidoskky commented 2022-08-17 18:29:22 +00:00
Outdated
Review
Copy Link

I'm not sure how the database will handle this.
This is correct as of now, moving this block after the else will generate errors if the else block is encountered since the fetchedItems variables is only defined within the if block.

I'm not sure how the database will handle this. This is correct as of now, moving this block after the else will generate errors if the else block is encountered since the fetchedItems variables is only defined within the if block.
AmineB commented 2022-08-17 18:35:26 +00:00
Outdated
Review
Copy Link

The else block would have the BD fetching that'll assign the DB items to fetchedItems.

The `else` block would have the BD fetching that'll assign the DB items to `fetchedItems`.
items = ArrayList(fetchedItems)
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 18:29:55 +00:00
Outdated
Review
Copy Link

This should be moved after the else block

This should be moved after the `else` block
}
} else {
// TODO: Provide an error message if the connection is not available.
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 14:11:48 +00:00
Outdated
Review
Copy Link

The error message should be displayed when the network status change, not after every api call.

Same for every comment like this one.

The error message should be displayed when the network status change, not after every api call. Same for every comment like this one.
// TODO: Get items from the database
}
return items
}
suspend fun getOlderItems(): ArrayList<SelfossModel.Item> {
// TODO: Check connectivity
val offset = items.size
val fetchedItems = api.getItems(displayedItems.type,
settings.getString("prefer_api_items_number", "200").toInt(),
offset,
tagFilter?.tag,
sourceFilter?.id?.toLong(),
searchFilter,
null)
if (isConnectionAvailable.value) {
val offset = items.size
val fetchedItems = api.getItems(
displayedItems.type,
settings.getString("prefer_api_items_number", "200").toInt(),
offset,
tagFilter?.tag,
sourceFilter?.id?.toLong(),
searchFilter,
null
)
if (fetchedItems != null) {
appendItems(fetchedItems)
if (fetchedItems != null) {
appendItems(fetchedItems)
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 18:30:20 +00:00
Outdated
Review
Copy Link

This should be moved after the else block

This should be moved after the `else` block
}
} else {
// TODO: Provide an error message
}
return items
}
suspend fun allItems(itemType: ItemType): List<SelfossModel.Item>? =
api.getItems(itemType.type, 200, 0, tagFilter?.tag, sourceFilter?.id?.toLong(), searchFilter, null)
suspend fun allItems(itemType: ItemType): List<SelfossModel.Item>? {
return if (isConnectionAvailable.value) {
api.getItems(
itemType.type,
200,
0,
tagFilter?.tag,
sourceFilter?.id?.toLong(),
searchFilter,
null
)
} else {
null
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 14:13:59 +00:00
Outdated
Review
Copy Link

This should be a todo

This should be a todo
}
}
private fun appendItems(fetchedItems: List<SelfossModel.Item>) {
// TODO: Store in DB if enabled by user
@ -97,35 +122,52 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
}
suspend fun reloadBadges(): Boolean {
// TODO: Check connectivity, calculate from DB
var success = false
val response = api.stats()
if (response != null) {
badgeUnread = response.unread
badgeAll = response.total
badgeStarred = response.starred
success = true
if (isConnectionAvailable.value) {
val response = api.stats()
if (response != null) {
badgeUnread = response.unread
badgeAll = response.total
badgeStarred = response.starred
success = true
}
} else {
// TODO: Compute badges from database
}
return success
}
suspend fun getTags(): List<SelfossModel.Tag>? {
// TODO: Check success, store in DB
return api.tags()
// TODO: Store in DB
return if (isConnectionAvailable.value) {
api.tags()
} else {
// TODO: Compute from database
null
}
}
suspend fun getSpouts(): Map<String, SelfossModel.Spout>? {
// TODO: Check success, store in DB
return api.spouts()
// TODO: Store in DB
return if (isConnectionAvailable.value) {
api.spouts()
} else {
// TODO: Compute from database
null
}
}
suspend fun getSources(): ArrayList<SelfossModel.Source>? {
// TODO: Check success
return api.sources()
// TODO: Store in DB
return if (isConnectionAvailable.value) {
api.sources()
} else {
// TODO: Compute from database
null
}
}
suspend fun markAsRead(item: SelfossModel.Item): Boolean {
// TODO: Check internet connection
val success = markAsReadById(item.id)
if (success) {
@ -135,11 +177,15 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
}
suspend fun markAsReadById(id: Int): Boolean {
// TODO: Check internet connection
return api.markAsRead(id.toString())?.isSuccess == true
var success = false
if (isConnectionAvailable.value) {
success = api.markAsRead(id.toString())?.isSuccess == true
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-20 18:34:15 +00:00
Outdated
Review
Copy Link

Every block that does something like

        var success = false
	
        if (isNetworkAvailable()) {
            success = DOSOMETHING()
        }
        return success

can be refactored to something like

return isNetworkAvailable() && DOSOMETHING()
Every block that does something like ``` var success = false if (isNetworkAvailable()) { success = DOSOMETHING() } return success ``` can be refactored to something like ``` return isNetworkAvailable() && DOSOMETHING() ```
}
return success
}
suspend fun unmarkAsRead(item: SelfossModel.Item): Boolean {
// TODO: Check internet connection
val success = unmarkAsReadById(item.id)
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 18:31:10 +00:00
Outdated
Review
Copy Link

This should be removed.

This should be removed.
if (success) {
@ -150,7 +196,11 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
suspend fun unmarkAsReadById(id: Int): Boolean {
// TODO: Check internet connection
return api.unmarkAsRead(id.toString())?.isSuccess == true
var success = false
if (isConnectionAvailable.value) {
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-20 18:34:32 +00:00
Outdated
Review
Copy Link

Please refactor this

Please refactor this
success = api.unmarkAsRead(id.toString())?.isSuccess == true
}
return success
}
suspend fun starr(item: SelfossModel.Item): Boolean {
@ -163,8 +213,11 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
}
suspend fun starrById(id: Int): Boolean {
// TODO: Check success, store in DB
return api.starr(id.toString())?.isSuccess == true
var success = false
if (isConnectionAvailable.value) {
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-20 18:34:42 +00:00
Outdated
Review
Copy Link

Please refactor this

Please refactor this
success = api.starr(id.toString())?.isSuccess == true
}
return success
}
suspend fun unstarr(item: SelfossModel.Item): Boolean {
@ -177,14 +230,19 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
}
suspend fun unstarrById(id: Int): Boolean {
// TODO: Check internet connection
return api.unstarr(id.toString())?.isSuccess == true
var success = false
if (isConnectionAvailable.value) {
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-20 18:34:48 +00:00
Outdated
Review
Copy Link

Please refactor this

Please refactor this
success = api.unstarr(id.toString())?.isSuccess == true
}
return success
}
suspend fun markAllAsRead(items: ArrayList<SelfossModel.Item>): Boolean {
// TODO: Check Internet connectivity, store in DB
val success = api.markAllAsRead(items.map { it.id.toString() })?.isSuccess == true
suspend fun markAllAsRead(items: ArrayList<SelfossModel.Item>): Boolean {
var success = false
if (isConnectionAvailable) {
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-20 18:35:04 +00:00
Outdated
Review
Copy Link

Please refactor this

Please refactor this
success = api.markAllAsRead(items.map { it.id.toString() })?.isSuccess == true
}
if (success) {
for (item in items) {
@ -233,45 +291,51 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
tags: String,
filter: String
): Boolean {
// TODO: Check connectivity
val response = api.createSourceForVersion(
title,
url,
spout,
tags,
filter,
apiMajorVersion
)
var response = false
if (isConnectionAvailable.value) {
response = api.createSourceForVersion(
title,
url,
spout,
tags,
filter,
apiMajorVersion
)?.isSuccess == true
}
return response != null
return response
}
suspend fun deleteSource(id: Int): Boolean {
// TODO: Check connectivity, store in DB
// TODO: Store in DB
var success = false
val response = api.deleteSource(id)
if (response != null) {
success = response.isSuccess
if (isConnectionAvailable.value) {
val response = api.deleteSource(id)
if (response != null) {
success = response.isSuccess
}
}
return success
}
suspend fun updateRemote(): Boolean {
// TODO: Handle connectivity issues
val response = api.update()
return response?.isSuccess ?: false
var response = false
if (isConnectionAvailable.value) {
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-20 18:36:21 +00:00
Outdated
Review
Copy Link

This should use the same isNetworkAvailable() function as the others.

This should use the same `isNetworkAvailable()` function as the others.
AmineB commented 2022-08-20 18:36:34 +00:00
Outdated
Review
Copy Link

And it can be refactored as the ones before.

And it can be refactored as the ones before.
response = api.update()?.isSuccess == true
AmineB marked this conversation as resolved Outdated
AmineB commented 2022-08-17 18:33:56 +00:00
Outdated
Review
Copy Link

It's weird to have this here. offlineOverride shouldn't be overridden from inside the repository.

If offlineOverride is true then return false.

There is no need for an exception for this method.

It's weird to have this here. `offlineOverride` shouldn't be overridden from inside the repository. If `offlineOverride` is `true` then return `false`. There is no need for an exception for this method.
}
return response
}
suspend fun login(): Boolean {
var result = false
try {
val response = api.login()
if (response != null && response.isSuccess) {
result = true
if (isConnectionAvailable.value) {
try {
val response = api.login()
result = response?.isSuccess == true
} catch (cause: Throwable) {
Napier.e(cause.stackTraceToString(), tag = "RepositoryImpl.updateRemote")
}
} catch (cause: Throwable) {
Napier.e(cause.stackTraceToString(),tag = "RepositoryImpl.updateRemote")
}
return result
}
@ -290,13 +354,14 @@ class Repository(private val api: SelfossApi, private val apiDetails: ApiDetails
}
private suspend fun updateApiVersion() {
// TODO: Handle connectivity issues
val fetchedVersion = api.version()
if (fetchedVersion != null) {
apiMajorVersion = fetchedVersion.getApiMajorVersion()
settings.putInt("apiVersionMajor", apiMajorVersion)
} else {
apiMajorVersion = settings.getInt("apiVersionMajor", 0)
apiMajorVersion = settings.getInt("apiVersionMajor", 0)
if (isConnectionAvailable.value) {
val fetchedVersion = api.version()
if (fetchedVersion != null) {
apiMajorVersion = fetchedVersion.getApiMajorVersion()
settings.putInt("apiVersionMajor", apiMajorVersion)
}
}
}