refactor if to takeIf and return "return" without label kotlin-CodePudding

I am trying to write more idiomatic Kotlin code and I am stuck with the best way to refactor this if condition. Basically when the condition if true (fragment is GenericActionsBottomSheetDialog instance in a list of Fragments) I return the funcion itself.

Here is what I had and how I refactored it. Is there better way to achieve it? After my refactoring it get worse:

Before refactor:

        supportFragmentManager.fragments.iterator().forEach {
            if (it is GenericActionsBottomSheetDialog)
                return

After refactor:

        supportFragmentManager.fragments.iterator().forEach { it ->
            it.apply {
            takeIf { it is FragmentType }?.apply { return }}}

CodePudding user response：

If this is the only thing in your current function (which it should IMO), you could get rid of the non-local return by using takeWhile instead:

supportFragmentManager.fragments
    .takeWhile { it !is GenericActionsBottomSheetDialog }
    .forEach {
         // do stuff 
    }

/!\ be careful that this changes semantics if there is other stuff after the forEach in the same function declared with fun.

If you expect many fragments in the list, you could also use asSequence() before takeWhile so you don't create an intermediate list.

CodePudding user response：

This is an opinion based question, and answers cannot be any different.

That being said: there is nothing wrong with if clauses. From what I can see from your current question, I'd leave it with an if.

Now, if you really do not want to use it, filter elements that are not of type GenericActionsBottomSheetDialog and apply whatever function you want on them (the part that is in your else clause, which we do not see).

CodePudding user response：

Here's one possibility, which separates the decision from the action:

if (supportFragmentManager.fragments.any{ it is GenericActionsBottomSheetDialog })
    return

I think this approach makes the intent clearest. (It's also about the most efficient.)

any() simply checks each item in turn, stopping when it finds a match (or when it reaches the end of the list). Kotlin has many functions like this (inspired by functional programming languages) that use lambdas to operate on lists and other structures. They tend to be named for what they do, rather than how they do it — which makes code using them both short and easy to read. (You should be writing code for people to read, as much as for computers to execute!)

For completeness, here's another approach, which uses filterIsInstance():

if (supportFragmentManager.fragments
        .filterIsInstance<GenericActionsBottomSheetDialog>)
        .isNotEmpty())
    return

There are bound to be many other ways. But I agree with the commenter that your ‘refactored’ approach, while using many more Kotlin functions, has little else to recommend it!

CodePudding user response：

@gidds solution is IMO the most idiomatic one:

if (supportFragmentManager.fragments
   .any { it is GenericActionsBottomSheetDialog }) return

I would like to add this solution eliminating the if:

supportFragmentManager.fragments
   .firstOrNull { it is GenericActionsBottomSheetDialog } 
   ?.run { return }

It's a matter of taste which one you pick, I prefer the first one.

I was wondering why you use the iterator? You could simply do:

supportFragmentManager.fragments.forEach {