How badly can a simple Kotlin function be written? ¶
An abomination lurking deep in a production codebase...
This is a real gem, found in the middle of a fairly large codebase for more than two years:
1: object SomeFileUtils { 2: fun getFilePath(context: Context, fileName: String, @RawRes resId: Int): String = 3: File(context.filesDir, fileName) 4: .let { file -> 5: if (file.canRead() && file.length() > 0) { 6: file.absolutePath 7: } else { 8: context 9: .resources 10: .openRawResource(resId) 11: .use { inputStream -> 12: file 13: .outputStream() 14: .use { outputStream -> 15: inputStream.copyTo(outputStream) 16: } 17: file.absolutePath 18: } 19: } 20: } 21: }
It's so wrong that it's almost beautiful… Almost.
The person who originally wrote this code must have been a little strange in the head, which isn't that unusual in this industry. The reviewer of this code who agreed to merge it - if they even existed - should probably reflect on how serious they are about doing their job, but well - it happens to the best of us. A single fuckup like this is nothing too unusual.
The real tragedy starts after that, though. Code is read much more often than it is written, so it had to be read by other programmers in the time it existed. Especially since many different programmers worked on a project, most of them only for a short time. Now, not one of these people took it upon themselves to eradicate this monstrosity, even though it would be 5 minutes of work. Here's how could it have looked like:
1: import com.companyname.utils.using 2: fun createFileForResource(context: Context, fileName: String, @RawRes resId: Int): File { 3: val file = File(context.filesDir, fileName) 4: if (file.canRead().not() || file.length() == 0L) { 5: val resource = context.resources.openRawResource(resId) 6: using(resource, file.outputStream()) { input, output -> 7: input.copyTo(output) 8: } 9: } 10: return file 11: }
I'll start by appealing to authority (because I'm just as lazy as those that will, no doubt, persecute me with Uncle Bob quotes):
Sometimes, the elegant implementation is just a function. Not a method. Not a class. Not a framework. Just a function. – John Carmack
I happen to agree, and it seemed like it was the case in this particular instance, so that's the first change to the code.
Switching to the block body was obvious, since that's the only way to declare vals in direct function scope, and it looked like giving a name to a subexpression could be helpful.
Flipping the condition not only highlighted the exact conditions (explicitly) in
which the function does something special, but also allowed the use of just an
if
statement and dropping the use of else
block. It's now clear that,
no matter what happens, this function will return a File object - there's only
one return
statement, and it's explicit, making it really hard to miss.
Simultaneously using more than one Closeable instance is a pretty
common thing to do, and deserves its own helper function. You can find using
implementation on Github - it's boring, schematic code, so writing it yourself
wouldn't be too fun. It's a single file liberally licensed utility that you can
just copy and paste to your utils module.
Finally, since stringly-typed code is an abomination that should be eradicated
from the face of the Earth, we return the File
object itself, rather then just
the path to it. The calling code knows better in what form it wants the file to
be: if it's for printing, file path is OK, but if you wanted to read it, you'd
have to create a new File
object based on the returned path. It's always
better to leave the representation decisions to the caller, and it pays off to
return the most universal representation you have access to.