[KotlinCleanup] utilize .apply in NavigationDrawerActivity.kt

[bennettj12]

Purpose / Description

Change small section to use kotlin .apply scope function

https://github.com/ankidroid/Anki-Android/issues/10489

How Has This Been Tested?

Tested on emulator

Checklist

Please, go through these checks before submitting the PR.

• [x] You have not changed whitespace unnecessarily (it makes diffs hard to read)
• [x] You have a descriptive commit message with a short title (first line, max 50 chars).
• [x] Your code follows the style of the project (e.g. never omit braces in if statements)
• [ ] You have commented your code, particularly in hard-to-understand areas
• [x] You have performed a self-review of your own code
• [ ] UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• [ ] UI Changes: You have tested your change using the Google Accessibility Scanner

[bennettj12]

I can't seem to replicate the test com.ichi2.async.CollectionTaskSearchCardsTest failure locally.

[Arthur-Milchior]

Which failure are you talking about? everythnig seems to work here

[bennettj12]

Which failure are you talking about? everythnig seems to work here

Ah, yesterday it had failed the windows unit test on github, seems to be fine now?

[Arthur-Milchior]

Which failure are you talking about? everythnig seems to work here

Ah, yesterday it had failed the windows unit test on github, seems to be fine now?

It is possible that someone restarted the test. Sadly some tests are flaky, so it may be the cause of it. I don't recall which one exactly, but that's not surprising that an async test would be flaky

[bennettj12]

Nice to meet you as well @Arthur-Milchior Thanks for letting me know, I was unsure after reading the kotlin cleanup thread whether it should be one PR per cleanup or on PR per class. But I see that even looking only at the issue of using .apply there is more that can be done.

I have some good experience with Java, but only just learning kotlin. Still, I think I understand enough that I can utilize .apply more in this class, so I will try to add to this first as suggested.

[bennettj12]

I have some questions about this: Is the use of .apply (and the other scope functions) purely for simplification/readability or is there more to it than that?

Like, if i were to change a single call of a method to use apply, mDrawerLayout!!.addDrawerListener(drawerToggle) -> mDrawerLayout!!.apply { addDrawerListener(drawerToggle) } My assumption right now is that this is a change that would serve no purpose.

Another thought is that there are several places where ?.apply could be used in place of a null check, I think this looks a little better, but I could understand that some might find it less readable.

if (supportActionBar != null) {
      supportActionBar!!.title = title
}

could be changed to

supportActionBar?.apply {
      this.title = title
}

Does this kind of change make sense, or is it also needless?

Thanks and hope you don't mind me asking what might be very trivial questions, that's just what I've been wondering so far.

[Arthur-Milchior]

To the best of my understanding, apply and other scope function are mainly here for readability purpose. However, there is a few exceptions to that. If you do x!!.apply{foo}, you only need to check that the value is non null a single time instead of checking it each time you access x. Also, some of the code do

objectVariable = expression
objectVariable.fun_1()
...
objectVariable.fun_n()

the trouble is that, since objectVariable is not a local variable, it is theoretically possible that another function access it and change its value if we were to use concurrency. So each access to it require to actually check which value is currently in the object, which is slightly costly. On the other hand

objectVariable = expression.apply {
  fun_1()
  ...
  fun_n()
}

mean that the result of expression is saved locally, and only once all fun_i's are called, the expression is assigned to objectVariable. Which is slightly more efficient. However, this mean that if you use asynchronous code to touch obejctVariable (or if one of the fun_i were to touch it) both behaviors are actually different). Without scope function, it would be equivalent to

val localValue = expression
localValue.fun_1()
...
localValue.fun_n()
objectVarible = localValue

However, I answer this for the sake of having a complete answer to your question. In practice, the time you're saving here is so little that you'll never notice it. You only need to care about it if this were in a function that were executed inside a loop, maybe a function that is called on each card, or on each note, during sync of database check. As an example, saving a milisecond by card when you have 10 thousands cards would mean saving ten seconds. However, saving a milisecond in the activity creation, which is called only once, is really not something we should try to optimize for. Furthermore, if we had to deal with concurrency issue, it would mean we have a huge problem with the architecture of this code, and apply may help to hide this problem, but in no case would it be an acceptable solution to get a proper architecture.

That is to say, as far as we are concerned here, yes, scope function are only here in order to improve the code readability

[lukstbit]

@bennettj12 Everything looks ok but please rebase your PR on top of main so we don't end up with an extra merge commit.

[github-actions[bot]]

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically