Skip to content
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

Refactor EuTxManager Java to Kotlin #244

Merged
merged 9 commits into from
Sep 19, 2022

Conversation

serim53
Copy link
Contributor

@serim53 serim53 commented Sep 10, 2022

Refactor EuTxManager java to kotlin

  • Convert Java style code to Kotlin style (Kotlin block function)
  • Change Handler to Coroutine Scope
  • Versionize AudioTrack
    • Apply Builder Pattern
  • Deprecate setMode() and convert into Kotlin format

@yeonns yeonns requested review from yeonns and designe September 10, 2022 08:03
Copy link
Member

@yeonns yeonns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serim53 Thank you for refactoring EuTxManager! It looks good 👍
One thing I wanna comment is that txCore is included at constructor. txCore is only used at EuTxManager internally, and its initialization is always same, so I think it would be removed at constructor. How about declaraing as private val?

And, EuTxManagerTest has unused imports, so how about removing it as well? 😄

@yeonns yeonns linked an issue Sep 12, 2022 that may be closed by this pull request
@yeonns yeonns linked an issue Sep 12, 2022 that may be closed by this pull request
@serim53
Copy link
Contributor Author

serim53 commented Sep 12, 2022

@serim53 Thank you for refactoring EuTxManager! It looks good 👍 One thing I wanna comment is that txCore is included at constructor. txCore is only used at EuTxManager internally, and its initialization is always same, so I think it would be removed at constructor. How about declaraing as private val?

And, EuTxManagerTest has unused imports, so how about removing it as well? 😄

As you said, txcore is only used internally, so I also think it's better to move it to the internal code! I corrected this part and deleted the import, and I commited it again! Thank you for checking carefully! 🥰

Copy link
Member

@designe designe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! @serim53
Sorry for the late riview 😢

I read all your EuTxManager.kt code.

  • By replacing Handler with corountine, android os dependency has disappeared, and flexibility seems to have been strengthened.
  • In addition, readability enhancement was noticeable through AudioTrack's Builder pattern.

Overall, the refactored code seems to be neatly written.
I leave some review-comments here :)

euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
@designe designe requested review from SeonJK and zion830 September 12, 2022 16:06
@SeonJK
Copy link
Member

SeonJK commented Sep 12, 2022

Great contribution!

I read your codes and have some questions.

In deprecated functions; setMode() and setCode(), does it also call txCore.setMode() and txCore.setCode()?

Copy link
Contributor

@zion830 zion830 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first Kotlin Transformation! 👍
I left some reviews about nullable :D

euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
@serim53 serim53 requested review from designe, yeonns and zion830 and removed request for SeonJK, designe and yeonns September 14, 2022 14:23
@designe
Copy link
Member

designe commented Sep 16, 2022

@zion830 Thanks for your helpful review :)
@serim53 It's almost done ;) after modificaiton about @Deprecated annotation, It's done!

@serim53
Copy link
Contributor Author

serim53 commented Sep 18, 2022

@zion830 Thanks for your helpful review :) @serim53 It's almost done ;) after modificaiton about @Deprecated annotation, It's done!

I finally modified all reviews! Thanks for your help!☺️

Copy link
Member

@yeonns yeonns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serim53 @zion830 Thank you for update! Now, it looks perfect!

Copy link
Member

@designe designe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ReplaceWith does not yet support the properties of getter and setter roles, it seems that there is currently only a way to display it with a message.
For the rest, the same expression was expressed more concisely.

@serim53 Thanks for your great migration & refactoring!

euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
euphony/src/main/java/co/euphony/tx/EuTxManager.kt Outdated Show resolved Hide resolved
@designe designe added the osca-22 Contribution From Open Source Academy 2022 label Sep 19, 2022
@designe designe merged commit 8870e23 into euphony-io:master Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement osca-22 Contribution From Open Source Academy 2022
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Refactoring] EuTxManager java to kotlin
5 participants