The Right Lang for the Job

Exploring the abbyss of non-mainstream programming languages

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.