mozilla-mobile / fenix

⚠️ Fenix (Firefox for Android) moved to a new repository. It is now developed and maintained as part of: https://github.com/mozilla-mobile/firefox-android
https://github.com/mozilla-mobile/firefox-android
Mozilla Public License 2.0
6.48k stars 1.27k forks source link

Bug 1814206 - Simplify the parameters of `DefaultTabsTrayController` #28778

Closed MozillaNoah closed 1 year ago

MozillaNoah commented 1 year ago

So sadly, only 6 of the parameters could be refactored out of the constructor at this time. We may be able to change this later if we do a sweeping refactor of TabsTrayFragment, as some of the remaining parameters are dependent upon logic passed-in as navigation arguments.

Pull Request checklist

QA

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-debug task.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Used by GitHub Actions.

Mugurell commented 1 year ago

Drive by: While I do appreciate classes / functions with less then 5 parameters (at the very max) passing each exact needed dependency is cleaner and support more easily maintainable code.

Going by the D in SOLID and thinking about how this controller should be reusable and not depend on lower level constructions we see that here the code gets tightly coupled to the activity and the accessors chain starting from it which is an anti-pattern since in it's ideal form this controller should not need to know about and hardcode the accessors chains to get it's needed dependencies. If we think about a dependency injection framework, why that is needed and how that works we'd also see that it would inject individual dependencies.

I remember when we did the opposite of this patch and refactored many components throughout the app to ensure they are portable and do not depend on outside modules - they don't need to know how a dependency is accessed / constructed in https://github.com/mozilla-mobile/fenix/issues/12565.

MozillaNoah commented 1 year ago

Drive by: While I do appreciate classes / functions with less then 5 parameters (at the very max) passing each exact needed dependency is cleaner and support more easily maintainable code.

Going by the D in SOLID and thinking about how this controller should be reusable and not depend on lower level constructions we see that here the code gets tightly coupled to the activity and the accessors chain starting from it which is an anti-pattern since in it's ideal form this controller should not need to know about and hardcode the accessors chains to get it's needed dependencies. If we think about a dependency injection framework, why that is needed and how that works we'd also see that it would inject individual dependencies.

I remember when we did the opposite of this patch and refactored many components throughout the app to ensure they are portable and do not depend on outside modules - they don't need to know how a dependency is accessed / constructed in #12565.

I think your comment makes sense. I'm going to cancel this PR in favor of re-examining the dependencies after (or perhaps during) the Tabs Tray to Compose effort, where maybe we can do more to clean up all of the lambdas passed-in and all of the business logic setup code inside of TabsTrayFragment.