Open BeQuietLee opened 5 years ago
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.
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.
Also in this class, L68, in function resetRepository()
, synchronization with lock
instead of this
may cause returning null in function provideTasksRepository(Context)
.
fun resetRepository() {
synchronized(lock) {
runBlocking {
FakeTasksRemoteDataSource.deleteAllTasks()
}
// Clear all data to avoid test pollution.
database?.apply {
clearAllTables()
close()
}
database = null
tasksRepository = null
}
}```
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)
?Thanks for your attention.