2
\$\begingroup\$

So, here's a task. A background thread may or may not call all of it's listeners some time after sendRequest() was called (in other words sendRequest() schedules a possible event). The required function must:

  1. Subscribe to those events
  2. Call sendRequest() periodically until the function returns anything
  3. Return the first suitable result (filtering is done in the listener and is omitted in this snippet) or null after a timeout
  4. Unsubscribe from those events regardless of the result
suspend fun findServer(): Server? {
    var listener: ((Server) -> Unit)? = null
    return try {
        withTimeoutOrNull(10000) {
            suspendCancellableCoroutine<Server> { continuation ->
                listener = { server: Server ->
                    continuation.resume(server)
                }.also { discoveryService.addListener(it) }

                launch {
                    while (continuation.isActive) {
                        discoveryService.sendRequest()
                        delay(1000)
                    }
                }
            }
        }
    } finally {
        listener?.let { discoveryService.removeListener(it) }
    }
}

The current implementation seems to work fine, but the nullable listener variable feels kind of clunky and requires using also to keep the listener reference, which in turn breaks type inference and makes me specify listener's parameter type (no big deal, but...). I could use a lateinit var here, but it probably can lead to exceptions when withTimeoutOrNull() stops before the listener was initialized.

Any recommendations on improving this snippet? Or am I, in fact, doing everything totally wrong here?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Think it's pretty good and you can't think of much else. Some tiny bits I can think of:

  • Make discoveryService parameter.
  • Do not use launch without specifying CoroutineScope.
\$\endgroup\$
2
  • \$\begingroup\$ Well, the application is built on dependency injection and discoveryService is an injected component so it shouldn't be a parameter. I'm not too comfortable with CoroutineScopes yet, but it seems that withTimeoutOrNull provides a scope. Should I wrap it somewhere? \$\endgroup\$
    – sibwaf
    Dec 10, 2019 at 14:45
  • \$\begingroup\$ Yeah I get it and I think you are right and withTimeoutOrNull indeed provides scope, my bad :) \$\endgroup\$
    – K.H.
    Dec 10, 2019 at 16:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.