refactor: Modularising the code base #291
Assignees
Milestone
Comments
nikclayton
added a commit
that referenced
this issue
Dec 4, 2023
The existing code base is a single monolithic module. This is relatively simple to configure, but many of the tasks to compile the module and produce the final app have to run in series. This is unnecessarily slow. This change starts to split the code in to multiple modules, which are: - :core:account - AccountManager, to break a dependency cycle - :core:common - low level types or utilities used in many other modules - :core:database - database types, DAOs, and DI infrastructure - :core:network - network types, API definitions, and DI infrastructure - :core:preferences - shared preferences definitions and DI infrastructure - :core:testing - fakes and rules used across different modules Benchmarking with gradle-profiler shows a ~ 17% reduction in incremental build times after an ABI change. That will improve further as more code is moved to modules. The rough mechanics of the changes are: - Create the modules, and move existing files in to them. This causes a lot of churn in import arguments. - Convert build.gradle files to build.gradle.kts - Separate out the data required to display a tab (`TabViewData`) from the data required to configure a tab (`TabData`) to avoid circular dependencies. - Abstract the repeated build logic shared between the modules in to a set of plugins under `build-logic/`, to simplify configuration of the application and library builds. - Be explicit that some nullable types are non-null at time of use. Nullable properties in types imported from modules generally can't be smart cast to non-null. There's a detailed discussion of why this restriction exists at https://discuss.kotlinlang.org/t/what-is-the-reason-behind-smart-cast-being-impossible-to-perform-when-referenced-class-is-in-another-module/2201. The changes highlight design problems with the current code, including: - The main application code is too tightly coupled to the network types - Too many values are declared unnecessarily nullable - Dependency cycles between code that make modularisation difficult Future changes will add more modules. See #291.
nikclayton
added a commit
that referenced
this issue
Dec 7, 2023
) The previous code generally started an activity by having the activity provide a method in a companion object that returns the relevant intent, possibly taking additional parameters that will be included in the intent as extras. E.g., if A wants to start B, B provides the method that returns the intent that starts B. This introduces a dependency between A and B. This is worse if B also wants to start A. For example, if A is `StatusListActivity` and B is`ViewThreadActivity`. The user might click a status in `StatusListActivity` to view the thread, starting `ViewThreadActivity`. But from the thread they might click a hashtag to view the list of statuses with that hashtag. Now `StatusListActivity` and `ViewThreadActivity` have a circular dependency. Even if that doesn't happen the dependency means that any changes to B will trigger a rebuild of A, even if the changes to B are not relevant. Break this dependency by adding a `:core:navigation` module with an `app.pachli.core.navigation` package that contains `Intent` subclasses that should be used instead. The `quadrant` plugin is used to generate constants that can be used to launch activities by name instead of by class, breaking the dependency chain. The plugin uses the `Activity` names from the manifest, so when an activity is moved in the future the constant will automatically update to reflect the new package name. If the activity's intent requires specific extras those are passed via the constructor, with companion object methods to extract them from the intent. Using the intent classes from this package is enforced by a lint `IntentDetector` which will warn if any intents are created using a class literal. See #291
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is a tracking issue for the work to split the code into multiple modules. This will:
The work is roughly:
LoginActivityto a feature module refactor: Modularise core activity classes, #393BaseActivityas well.core:designsystemto hold key design resourcesfeature:lists#537With "Everything else" to be fleshed out more after the initial split. My notes from previous discussions about similar work follow below.
Under this model we would have some core modules; at least types, database, network, designsystem, service (?), maybe one or two more.
And then the rest of the modules are organised around screens or complete features that the user uses.
So there'd be a hypothetical :screen:trending module that contains all the activity / fragment / viewmodel / resources / etc necessary to display the "Trending" screen, and that has dependencies on the relevant :core modules.
Thinking about these as screens or features mean that we don't fall in to the trap of trying to organise everything to mirror Android's class hierarchy.
There is a sweet spot where the module tree is of sufficient breadth and depth.
Too narrow and deep and we end up having to recompile too many things on every change. Too wide and shallow and too many unnecessary things get recompiled when a single module changes. Either way we lose the benefits of this sort of change.
A good rule of thumb is probably "What's the largest unit of UI that the user interacts with".
For example, there's currently
AboutActivityandLicenseActivity. I could imagine a:screen:aboutmodule that contains both of those.Or search -- that's multiple fragments (and an activity and viewmodel) -- but that would be in a
:screen:searchmodule, not three different modules like:screen:searchHashtag,:screen:searchAccounts,:screen:searchStatuses.The text was updated successfully, but these errors were encountered: