Tech-improvement: Refactor high-complexity classes

[psh]

Address the very high cyclomatic complexity we are seeing from Codacy by breaking up Activities & Fragments according to one of the well-respected architectural patterns (MVP / MVVM / etc)

[maskaravivek]

As part of #1092, we are looking to introduce a consistent pattern for our backend codebase. The same consistency should be followed for our presentation layer. There are several architectures out there and some of the most popular ones are MVP and MVVM. We already use MVP for our uploads flow and also for displaying campaigns(thanks to @ashishkumar468).

Am adding linkings to a few insightful blog posts which talk about how and why to use MVP in android.

• https://medium.com/cr8resume/make-you-hand-dirty-with-mvp-model-view-presenter-eab5b5c16e42
• https://android.jlelse.eu/architectural-guidelines-to-follow-for-mvp-pattern-in-android-2374848a0157
• https://www.raywenderlich.com/7026-getting-started-with-mvp-model-view-presenter-on-android

IMO, our app would benefit from following a standard architectural pattern for its presentation layer mainly because it would provide a clear understanding of structuring code and avoiding code smells to all contributors.

Some of the anti-patterns that I observe and hope to fix as a team is:

• not concerning yourself the lifecycle of Fragment and Activity. This leads to crashes, most commonly because of null context.
• referencing view elements of a particular Fragment or Activity directly in a different fragment or activity. This makes it harder to decouple different fragments.
• defining public methods in a fragment that is referenced from another activity or fragment. Ideally, we should use interfaces and listeners instead of calling public methods directly.
• a Fragment or Activity have high cyclomatic complexity. Ideally, these classes should have clean public interfaces and any communication between these elements should be through proper channels.
• A lot of business logic is present in the view layers. These should be extracted out to the presentation layer. Having things in the view layer itself leads to code duplication as well.

[misaochan]

There seems to be a bit of a duplicate here. I was intending for #1092 to be the main issue for this task, with #1863 for reference. Shall I close this issue and move your comment over to #1092 , @maskaravivek ?

[maskaravivek]

Yes I agree #1092 should be the main issue but it might get messy to have all the discussion on that thread. I thought it would be easier to follow the discussion if we have separate issues for mediawiki apis(retrofit and okhttp), shared prefs, presentation layer, content providers etc, keeping #1092 as the parent issue.

Edit: If you intend to keep the discussion at one place, feel free to close this issue and we can continue the discussion on #1092. We can probably crease separate issues while actually implementing the changes. :)

[misaochan]

Sorry for the delay in response, @maskaravivek . I just went through the articles on MVP and it looks good to me - I especially like that it benefits testability and reusability. :)

I would be on board with using this architecture for our backend overhaul. If anyone disagrees or would like to propose a different one, now is the time to speak! ;) The official announcements for approved grants will be out in a week's time, and Vivek has requested to start on this task ahead of time if possible.

[ashishkumar468]

I agree with @misaochan on the benefits. Even my vote goes for MVP.

[misaochan]

As there are no contradicting opinions, we will be going with MVP. :)

In order to maintain consistency in our codebase, we are thinking of requiring that all new PRs created after a certain date should adhere to this architecture, as well. This would apply to all volunteers, interns, and grantees - since there is no purpose in doing this overhaul if we are not going to maintain its consistency. Of course, by its very nature, this is unlikely to affect bugfixes and very small enhancements (for instance, if you just fix a NPE, we are not going to expect that you will refactor all the code surrounding it) - it is only larger changes and large enhancements that would be affected.

If no one has issues with this, I will create the rule for all PRs created after 18th March, and update our wiki. This may also tie in with new contribution guidelines if we reach a consensus at #2553

[misaochan]

Added a rule to our wiki for this: https://github.com/commons-app/apps-android-commons/wiki/Code-style