-
Notifications
You must be signed in to change notification settings - Fork 115
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
refactor: centralize Android NFC method exception handling #169
Conversation
Thanks! I am actually thinking about some more elegant ways to handle all possible exceptions. Something like: fun handleException(ex: Exception, result: Result?, desc: String) {
Log.e(TAG, "$desc error", ex)
val excMessage = ex.localizedMessage
when (ex) {
is IOException -> result?.error("500", "Communication error", excMessage )
is SecurityException -> result?.error("503", "Tag already removed", excMessage)
// some more types
else -> result?.error("510", "Unhandled error", excMessage) // I choose this code just randomly.
}
} Then it could be called from all |
With the current amount of I suppose refactoring the exception handling wouldn't take too much time and would definitely make things a lot cleaner. My main concern is I can't physically test the other tag types to ensure the behavior triggering the exception doesn't differ slightly. |
AFAIK Android NFC APIs mainly throw:
There are additional Seems it's safe to refactor, since no two different APIs throws the same exceptions except |
f115a3e
to
27fe9ba
Compare
Good points, I went ahead and offloaded it into more or less the copy-pasted version of your suggestion :) |
Yeah! This looks much better now. I just found that it could be merged into private fun runOnNfcThread(result: Result, desc: String, fn: () -> Unit) {
val wrappedFn = Runnable {
try {
fn()
} catch (ex: Exception) {
handleException(ex, result, desc)
}
}
if (!nfcHandler.post(fn)) {
result.error("500", "Failed to post job to NFC Handler thread.", null)
}
} Then:
By the way, there is an extra space at: flutter_nfc_kit/android/src/main/kotlin/im/nfc/flutter_nfc_kit/FlutterNfcKitPlugin.kt Line 70 in 27fe9ba
Will you please make the changes, and I'm really happy to merge. |
27fe9ba
to
a59477d
Compare
Oh! That's a lot better, agreed. I ended up moving Resolved the other two mentions as well. |
android/src/main/kotlin/im/nfc/flutter_nfc_kit/FlutterNfcKitPlugin.kt
Outdated
Show resolved
Hide resolved
a59477d
to
18e4684
Compare
Reduces code duplication and improves readability.
Fixes #168