Base 64 Kotlin extensions#406
Base 64 Kotlin extensions#406jaredsburrows wants to merge 2 commits intoandroid:masterfrom jaredsburrows:jb/base64-extensions
Conversation
| import java.nio.charset.Charset | ||
|
|
||
| /** | ||
| * From [String] to Base-64 encoded [String]. |
There was a problem hiding this comment.
This is a sentence fragment. Prefix it with a verb like "Convert".
| offset: Int = 0, | ||
| len: Int = this.length, | ||
| flags: Int = Base64.DEFAULT | ||
| ): String = Base64.encode(toByteArray(), offset, len, flags).toString(Charset.forName("US-ASCII")) |
There was a problem hiding this comment.
The toByteArray charset parameter should be hoisted onto this function as we cannot assume UTF-8 is the desired encoding. And below.
| offset: Int = 0, | ||
| len: Int = this.length, | ||
| flags: Int = Base64.DEFAULT | ||
| ): String = Base64.encode(toByteArray(), offset, len, flags).toString(Charset.forName("US-ASCII")) |
There was a problem hiding this comment.
Use http://kotlinlang.org/api/latest/jvm/stdlib/kotlin.text/-charsets/-u-s_-a-s-c-i-i.html instead of looking up charset by name. And below.
| import java.nio.charset.Charset | ||
|
|
||
| /** | ||
| * From [String] to Base-64 encoded [String]. |
There was a problem hiding this comment.
Also nit: lowercase "Base" on all these
| /** | ||
| * From Base-64 encoded [String] to decoded [String]. | ||
| */ | ||
| fun String.fromBase64( |
There was a problem hiding this comment.
The Kotlin standard library typically puts "from" methods as extensions on the companion of the type on which they operate and take an instance as the first parameter.
There was a problem hiding this comment.
There is no companion object in ByteArray.
| /** | ||
| * From Base-64 encoded [ByteArray] to decoded [ByteArray]. | ||
| */ | ||
| fun ByteArray.fromBase64( |
There was a problem hiding this comment.
These can all be inline since they're aliases.
| assertEquals(expected, actual) | ||
| } | ||
|
|
||
| @Test fun fromBase64String() { |
There was a problem hiding this comment.
pair of byte array tests too?
|
@JakeWharton All fixed. What do you want to do about |
|
We might have to leave it out until https://youtrack.jetbrains.com/issue/KT-11968 is fixed. I'll ask JetBrains about it at our weekly meeting next week. |
JakeWharton
left a comment
JakeWharton
left a comment
There was a problem hiding this comment.
Worked out an API design that I like. Sorry for the churn.
| len: Int = this.length, | ||
| flags: Int = Base64.DEFAULT, | ||
| charset: Charset = Charsets.US_ASCII | ||
| ): String = Base64.encode(toByteArray(), offset, len, flags).toString(charset) |
There was a problem hiding this comment.
The charset should be used for toByteArray and the default should be UTF_8. The ASCII charset is for toString().
| /** | ||
| * Convert [String] to base-64 encoded [String]. | ||
| */ | ||
| inline fun String.toBase64( |
There was a problem hiding this comment.
Let's call this .encodeBase64 as there's no Base64 type so we're not really going "to" it.
| /** | ||
| * Convert base-64 encoded [String] to decoded [String]. | ||
| */ | ||
| inline fun String.Companion.fromBase64( |
There was a problem hiding this comment.
This can go back to being an instance extension and be called decodeBase64().
| len: Int = input.length, | ||
| flags: Int = Base64.DEFAULT, | ||
| charset: Charset = Charsets.US_ASCII | ||
| ): String = Base64.decode(input.toByteArray(), offset, len, flags).toString(charset) |
There was a problem hiding this comment.
Pass US_ASCII to toByteArray.
| offset: Int = 0, | ||
| len: Int = input.length, | ||
| flags: Int = Base64.DEFAULT, | ||
| charset: Charset = Charsets.US_ASCII |
| */ | ||
| inline fun String.toBase64( | ||
| offset: Int = 0, | ||
| len: Int = this.length, |
There was a problem hiding this comment.
Length won't work here as you are using it as the length of the bytes whereas the parameter represents the number of characters. You need to do a substring before calling toByteArray and then pass the length of the byte array to Base64.
I would also write a test for this with two multi-byte UTF-8 characters in the source string and then a length of 1 which should thus encode more than one byte.
There was a problem hiding this comment.
I updated this PR with everything else.
How would this substring work?
There was a problem hiding this comment.
Any update on how you want this to work? Or do you mind if I just make the PR for the String to Base64 and Base64 to String?
2 participants
@romainguy