-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add startsWithAny and endsWithAny #3577
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: ByteZ1337 <0xbytez@gmail.com>
Signed-off-by: ByteZ1337 <0xbytez@gmail.com>
Why not just use Also, the name "startsWithAny" is confusing, in my opinion the semantics could be better if it were |
The same reason you use Collection#addAll instead adding every element with forEach. Methods like these are just way more convenient to use and make the code way more readable than using obscure lambdas in if statements.
How so? I think it's pretty easy to understand. I don't really see an issue with the name since there are other methods called "any" in the Standard Library.
I think the receiver should stay CharSequence since the actual string is checked. Again I use Collection#addAll not Collection#addAllTo |
Nope, that's not the case at all. having that method helps with increasing performance, for example if it were an Also, I don't see why this function should be in the standard library. All the functions from kotlin.collections.Collections.kt / kotlin.text.Strings.kt contain their own implementations, not one-liners. |
Sill makes it way more readable than obscure lambda that you don't instantly understand.
What about the empty and null checks? Those could also easily be implemented but still got into the standard library. And I also wouldn't call it its own function. It's just an extension of startsWith. With your logic methods like substringAfter would also be one liner functions.
Well obviously you can implement everything yourself in Strings.kt. Iirc these functions exist so you don't have to implement them every time you need them. I really don't understand why you're fighting this so much? They are just 2 functions that are useful and don't make any major changes to the standard library. |
I am not fighting this, I am asking why this should be in the standard library. If you could explain your use case it would be great. Also, looking at contributing.md, you did not:
Null checks are helpful because when using the value from elvis/if expression you need to use parenthesis.
How is it more readable? How is
The names are confusing, you think they are easy to understand because you know what they do. By just looking at the function names from the title, I had no idea what these functions are for. |
I think the function names are easy to understand and improve readability greatly. If you just skim through a program the method string.startsWithAny tells you faster and more clearly what is does compared to listStrings.any(string::startsWith) |
First of all, if you're neutral and just curious about why this should be implemented why did you dislike my post? For my use cases :
Sorry if I missed it but I never saw you explicitly asking for these? I didn't open an issue or talk to the employees because I thought 2 small functions would just waste their time. So I just opened this pull request and see if it gets merged. If this was wrong I would like to apologize. I did however not see the part with sample code which I will probably add to this pull request tomorrow. And I don't know what your "educated guess" is supposed to mean? (Getting a bit personal there huh) I did build it by following the instructions in the readme. But I don't understand why I should build my own forked standard library. Since I mostly do open source stuff this wouldn't make sense. Also, as you mentioned in your second comment I can just implement it myself which would make building the whole standard library a bit overkill. Also, if you find these names confusing you should probably open an issue to rename CharSequence#indexOfAny. |
Checking file extensions: val extensions = listOf("txt","exe")
val file = File(...)
file.extension in extensions
val fileName = "foo.bar"
fileName.substringAfterLast('.', "") in extensions Parsing a file: I do not think that is the average use case. |
Or path.endsWithAny("exe", "txt", ignoreCase = true) Which is way nicer to look at and supports ignoreCase. |
This comment has been minimized.
This comment has been minimized.
So I ran println("HashSet check: " + measureTimeMillis {
val extensions = hashSetOf("exe", "json", "xml", "bin", "kt", "jar", "txt")
val file = File("test.txt")
file.extension.toLowerCase() in extensions
}) and println("endsWithAny: " + measureTimeMillis { "test.txt".endsWithAny("exe", "json", "xml", "bin", "kt", "jar", "txt", ignoreCase = true) }) independently from each other and got these results (average of 10 runs):
I ran this code on play.kotlinlang.org to make sure it's not because of my hardware or some other stuff. When adding more extensions to the list both functions take longer at a pretty equal rate. So the time complexity seems to be O(n) for both methods. Here is the link if you want to try it yourself. Just uncomment the version you want to try. |
This comment has been minimized.
This comment has been minimized.
benchmark is rigged |
This comment has been minimized.
This comment has been minimized.
Well mine shows the actual time it takes to run, not some benchmark |
Also javadoc has tons of spelling mistakes you might wanna fix |
Something other than the missing plural? I copied the doc from the other methods and just modified it a bit. |
This comment has been minimized.
This comment has been minimized.
Still, why shouldn't mine be perfectly ok? It's the time it takes to execute the tasks. Also if its rigged then it means nothing either way (Don't have time to check rn). |
This comment has been minimized.
This comment has been minimized.
Looking and running your benchmarks they seem fine. (You use startsWith instead of ends if but adjusting the string length to account for that doesn't change the speeds too much). I still would like to have that functions, maybe just with more efficient methods. (This will be hard to implement because your methods can only be used for checking file extension and not general string where you don't have a delimiter) |
How am I not respecting your opinion? I'm just saying that I don't see anything wrong with my tests and that someone said that yours might be rigged? I never accused you of anything. It just doesn't make sense why these benchmarks would show different results. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Isn't that what "bigotry" means? English isn't my native language sorry. |
Just some functions I would find useful.