✨ Refactor Cache to be an interface #41
Conversation
fuse-android/src/test/java/com/github/kittinunf/fuse/android/FuseByteCacheTest.kt
Show resolved
Hide resolved
| @@ -30,16 +30,20 @@ object CacheBuilder { | |||
| } | |||
| } | |||
|
|
|||
| fun <T : Any> Config<T>.build() = Cache(this) | |||
| fun <T : Any> Config<T>.build(): Cache<T> = CacheImpl(this) | |||
There was a problem hiding this comment.
The caller of build() should not need to know what specific implementation of Cache is used. So this was changed to just return the interface type.
|
|
||
| class Cache<T : Any> internal constructor(private val config: Config<T>) : Fuse.Cacheable, | ||
| Fuse.Cacheable.Put<T>, Fuse.Cacheable.Get<T>, Fuse.DataConvertible<T> by config.convertible { | ||
| enum class Source { |
There was a problem hiding this comment.
Moved out of the concrete class, to the top level. This makes reusing of this enum more natural in alternative concrete implementations of Cache.
| MEM, | ||
| DISK, | ||
| } | ||
| interface Cache<T : Any> : Fuse.Cacheable, Fuse.Cacheable.Put<T>, Fuse.Cacheable.Get<T>, |
There was a problem hiding this comment.
This is the new interface definition.
| interface Cache<T : Any> : Fuse.Cacheable, Fuse.Cacheable.Put<T>, Fuse.Cacheable.Get<T>, | ||
| Fuse.DataConvertible<T> | ||
|
|
||
| class CacheImpl<T : Any> internal constructor( |
There was a problem hiding this comment.
Your concrete class now is of type Cache.
| if (diskCache.get(safeKey) == null) { | ||
| val converted = convertToData(value as T) |
There was a problem hiding this comment.
This line was moved into the if condition, because the convertToData operation is potentially expensive. Users of your library supply the converter, so "we" can't make assumptions about how fast it is.
It seems better to defer the data conversion until we absolutely know we need to do it. Previously, the conversion would have run even if diskCache.get(safeKey) returned a non-null value.
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
============================================
- Coverage 80.76% 80.21% -0.55%
+ Complexity 68 46 -22
============================================
Files 11 11
Lines 182 182
Branches 30 31 +1
============================================
- Hits 147 146 -1
- Misses 17 18 +1
Partials 18 18
Continue to review full report at Codecov.
|
|
I don't see anything wrong with these changes! It makes it even more flexible .. 👍 |
kittinunf
changed the title
|
Thanks @kittinunf! Would it be possible to cut a new release with these changes? |
|
Definitely! I will do that right away :) |
|
Done sir! ⏫ Please give it a try and report back, whether it works on your end ❤️ |
|
Thank you! |
…tions
In reference to this issue: #40
What is the problem you are trying to solve?
Per issue #40, reading from the cache is not as fast as it could be in certain scenarios. There is logic that both reads from, and writes to, the disk cache even when the cached data is readily available from memory. This drastically slows down what should otherwise be a fast fetch of data.
The reason for the disk read and write in this scenario ^ makes sense. You are making sure the disk LRU is updated to reflect the recent access to a cached entry.
I think, ideally, such updates would be non-blocking (performed asynchronously). I actually started down this path. The contract requirement to return a
Result, however, halted my progress. If a caller issues a cacheputexpecting aResult, the only two options areResult.SuccessorResult.Failure. How could aResultbe returned immediately, if the disk cache update is happening asynchronously? It can't. You would instead, perhaps, need aResult.Pendingor something along those lines.I felt your current implementation had contract obligations that I wasn't ready to upset. After all, you may already have clients that actually depend on the disk updates being synchronous. I would love to discuss this further, if you are open to contractual changes that might incorporate asynchronous disk cache updates as standard.
Instead, my strategy for this PR was to simply make it possible to have an alternate implementation of the
Cacheclass. This would allow myself, or others, to create their own implementation that chooses a different disk write strategy.What changed specifically?
Cacheclass was renamed toCacheImpl.Cacheinterface was introduced, andCacheImplchanged to implement that.Sourceenum was moved to be a top-level construct for other classes (such as my own implementation [yet to be written]) to use.CacheImpl.Sourcewere changed to just beSource.Anything else?
I confirmed that your tests all still pass. I also made sure the sample app was still working.