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

Inline Intent helper methods #1274

Merged
merged 9 commits into from
Oct 8, 2019
Merged

Inline Intent helper methods #1274

merged 9 commits into from
Oct 8, 2019

Conversation

veyndan
Copy link
Collaborator

@veyndan veyndan commented Oct 7, 2019

Inline Intent.get___Extra(String) methods as:

  • Logic needs to be duplicated between BaseFragment and BaseActivity to continue using this approach, reducing code maintainability.
  • The helper methods in BaseFragment always uses the null permissible getActivity() when the logical flow may require the use of the non-null assertive requireActivity().
  • Calling getIntent().get___Extra(String) is a pretty standard practice, and makes it known that the get___Extra(String) is being obtained by the intent that the started the activity.
  • BaseActivity#getIntExtra(name: String) returns -1 if the extra name is absent. By covering this up in a layer of abstraction is a potential source of difficult to find bugs.

@veyndan veyndan changed the title Base intent extra Inline Intent helper methods Oct 7, 2019
@Meisolsson
Copy link
Contributor

Why do we use activity?.intent? in app/src/main/java/com/github/pockethub/android/ui/gist/GistFileFragment.kt but activity.intent in app/src/main/java/com/github/pockethub/android/ui/commit/CommitCompareListFragment.kt?

@veyndan
Copy link
Collaborator Author

veyndan commented Oct 8, 2019

In the original app/src/main/java/com/github/pockethub/android/ui/gist/GistFileFragment.kt we don't assert that the activity should be non-nullable, so I have kept the logic the same by using activity?. In the original app/src/main/java/com/github/pockethub/android/ui/commit/CommitCompareListFragment.kt we assert that activity should never be null, so in the later lines activity is never null. I haven't changed the nullability contract in this PR.

@Meisolsson
Copy link
Contributor

Alright, that makes sense. I'll check this in a bit.

@Meisolsson Meisolsson merged commit 1d21487 into pockethub:master Oct 8, 2019
@veyndan veyndan deleted the base-intent-extra branch October 8, 2019 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants