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: Reduce Return Statements Across Multiple Functions #191

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

KesharwaniArpita
Copy link
Contributor

Contributor checklist


Description

This pull request removes the ReturnCount rule in detekt.yml, refactors several functions in the codebase to reduce the number of return statements. The aim is to improve readability, maintainability, and consistency in error handling while ensuring that each function maintains its original functionality. The changes ensure a maximum of two return points for better flow and clarity.

Changes Made

  1. Reduced return statements in various functions to a maximum of two, improving the overall readability and maintainability of the code.
  2. Consolidated logic to ensure consistent error handling and flow control while preserving existing functionalities.
  3. Enhanced clarity by restructuring conditions and using local variables to store results before returning.

Reducing the number of return statements in functions enhances code quality by:

  1. Making the flow of logic easier to follow.
  2. Reducing the chances of unintentional exits from functions.
  3. Enhancing the ability to maintain and extend the code in the future.

Please review the changes and provide any feedback or suggestions for further improvement. Your input is greatly appreciated!

Related issue

Copy link

github-actions bot commented Oct 13, 2024

Thank you for the pull request!

The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Android rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • The linting and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis andrewtavis requested review from angrezichatterbox and andrewtavis and removed request for angrezichatterbox October 13, 2024 11:08
@andrewtavis andrewtavis added the hacktoberfest-accepted Accepted as a part of Hacktoberfest label Oct 13, 2024
@KesharwaniArpita
Copy link
Contributor Author

Hi @andrewtavis ,

I managed to solve the following lint issues from earlier:

  1. Unused private functions (saveTreeUri, isProperSDRootFolder)
  2. Missing new lines at the end of files (Activity.kt, ContextStorage.kt)
  3. Unused parameter (parentDocumentFile)
    However, after fixing those, I now encountered these Detekt warnings:

"TooGenericExceptionCaught" in Activity.kt (line 87) and ContextStorage.kt (line 427).
I’ve started catching more specific exceptions like IOException, but I’m still unsure if I’ve covered all the possible cases. Could you suggest the best approach to handle this? Do you have any suggestions or guidance on how I should refine the exception handling?

Thanks for your help!

@andrewtavis
Copy link
Member

CC @angrezichatterbox: Any suggestions here? @KesharwaniArpita, let us work on this from here, thanks :)

@KesharwaniArpita
Copy link
Contributor Author

Sure!! Thanks for your support here @andrewtavis and @angrezichatterbox . Is there anything I could've done better here?

val trimmedText = textBeforeCursor.trim()
val lastChar = trimmedText.lastOrNull()
return lastChar != '.'
val textBeforeCursor = inputConnection.getTextBeforeCursor(Int.MAX_VALUE, 0).orEmpty().trim()
Copy link
Member

@angrezichatterbox angrezichatterbox Oct 20, 2024

Choose a reason for hiding this comment

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

Instead of using isEmpty(), I recommend performing a simple null check in the return statement. This adjustment should help avoid any errors.

Additionally, I suggest running the application in Android Studio or following the steps outlined in the new PR template, specifically ./gradlew lintKotlin detekt test. This could help catch potential issues early on. If you are facing any difficulties in running the application do tell. Happy to help :)

Thank you!

}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Could the comment be opened and closed properly?

Copy link
Member

Choose a reason for hiding this comment

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

I'll fix this and the above and we'll see what the state of it all is :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted as a part of Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants