Architecture-samples: [master] ServiceLocator.kt redundant code ?

[BeQuietLee]

In app/src/mock/java/com/example/android/architecture/blueprints/todoapp/ServiceLocator.kt，L43. Why repeat tasksRepository ?: in return?

Can the return be simplified as return tasksRepository ?: createTasksRepository(context) ?

    fun provideTasksRepository(context: Context): TasksRepository {
        synchronized(this) {
            return tasksRepository ?: tasksRepository ?: createTasksRepository(context) // HERE!
        }
    }

Thanks for your attention.

[0neel]

I suppose that's an attempt to implement Double-check locking pattern in idiomatic Kotlin-style way. Unfortunately, it's implemented incorrectly since the first null check should be done outside of synchronized block - that's required to avoid synchronization when the property is inited. Otherwise, one of the null-checks is useless.

[0neel]

Actually, it's possible to make it work correctly:

    fun provideTasksRepository(context: Context): TasksRepository {
        return tasksRepository ?: synchronized(this) {
            tasksRepository ?: createTasksRepository(context)
        }
    }

I wouldn't say this improves readability though.

[TchaikovDriver]

Also in this class, L68, in function resetRepository(), synchronization with lock instead of this may cause returning null in function provideTasksRepository(Context).

kotlin fun resetRepository() { synchronized(lock) { runBlocking { FakeTasksRemoteDataSource.deleteAllTasks() } // Clear all data to avoid test pollution. database?.apply { clearAllTables() close() } database = null tasksRepository = null } }