Reduce complexity - refactor existing code to use unified APIs

[misaochan]

As posted by @dbrant at #1489 :

I generally like to encourage a greater emphasis on QA, continual dogfooding, and reducing complexity (i.e. refactoring existing code to use unified APIs), rather than adding further complexity just for the purpose of capturing more detailed logs in the field. Based on a quick glance at the current state of the Commons app, it looks like there's a rather high amount of complexity and dependencies (to the point of requiring multidex? 😮)

For example, for the purpose of displaying images, you seem to be using Fresco and Glide and Picasso in different places, literally all three major competing image libraries for Android. This introduces a huge amount of bloat, as well as a threefold increase in the surface area for bugs to occur. Not to derail this particular thread, but my earnest recommendation would be to take a step back, establish some best practices for the architecture of the product, and work towards refactoring all of the code to adhere to each of these best practices, one at a time.

For instance:

Decide on a single image library (e.g. Fresco) and use it throughout the app.
Decide whether to use RxAndroid or plain AsyncTasks, and use them consistently (preferably RxAndroid), but don't use both.
Stop using the long-deprecated org.mediawiki:api library, and use Retrofit (with RxAndroid) for your network calls.
Evaluate whether you really need all the third-party components and libraries that you're depending on. They are all potential sources of bugs. (For example fr.avianey.com.viewpagerindicator or in.yuvi:http.fluent, both of which have been abandoned for six years)
Try not to use third-party libraries that promise to "simplify" things, like a library that "simplifies" populating a RecyclerView, or a library that "simplifies" asking for runtime permissions. They are all sources of bugs, which are out of your control.
Explain these best practices to newcomer volunteers, and reject pull requests that increase complexity unnecessarily.
Once you do this, the bugs that seem puzzling today will become more and more obvious, or will simply disappear automatically. Sorry for the long-windedness (and apologies if I'm overstepping), and let me know if I can help in any way.

This makes a lot of sense, thanks! I agree that we really do need to sit down and have a discussion about pruning our dependencies. We tend to allow volunteers to add libraries as long as it doesn't introduce any bugs at the time of testing or increase the APK size too much, but indeed we should probably tighten up on that if we want to have a more polished app, due to the increased maintenance required and the increased potential for bugs down the road. An overhaul of the legacy code is also certainly needed.

I would like to introduce this refactoring into our plans for 2019. :) However, we still need to talk about what exactly we want to prune, why we are pruning it, and how to go about it. So, I would invite everyone to chime in with their thoughts on:

1. Which libraries do you think we can remove? In cases of redundancy, which library would you recommend we use over the others and why?
2. What sort of guidelines should we use to determine whether a new library should be added or not, in the future?
3. What other refactoring do you think we urgently need to do?

Once we arrive at a consensus, I can draft this into our 2019 plans.

@maskaravivek @neslihanturan @nicolas-raoul @whym @psh

[sivaraam]

What sort of guidelines should we use to determine whether a new library should be added or not, in the future?

I guess that's not difficult. Avoid any library that can be. IOW, become dependent on a library only if we cannot reasonably work around the problem without the library and the user experience becomes terrible.

Of course these are just my opinions. Correct me if I'm wrong.

[nicolas-raoul]

Redundancy is unquestionably bad, we all agree about this already.

The app size is definitely important as many of our users are on slow networks.

But there are things that we should not implement ourselves, for instance image recognition features.

Runtime permissions strikes me as a typical example of an area where a popular library can drastically reduce the amount of code, bugs and maintenance on our side.

[maskaravivek]

Moreover, proper usage of proguard can considerably help in reducing the bloating caused by a particular library.

[dbrant]

Hey all, to follow up on the initial comments, here are some additional thoughts, questions, and points for discussion:

• Now that some of the excess complexity will be removed by Remove dependency on Glide, Picasso, SVG, and multidex. #1859, I would strongly recommend prioritizing the elimination of the woefully outdated mediawiki:api library and replacing it with Retrofit, using the json API, coupled with RxJava (This will also remove the need for the outdated yuvi:http.fluent and org.apache.http.legacy libraries if I'm not mistaken). I see you have an open pull request where a volunteer takes some first steps towards this refactor, but the volunteer seems to have dropped off. If you like, I could take it the rest of the way, as part of my 10% time at work. ;)

Regarding other unnecessary libraries:

• Yes, I know I'm rather harsh on libraries that give you all kinds of custom functionality, or simplify interactions with the framework, but that's only because of how many times I've been burned by them. These libraries are written by mortals, and are bound to contain bugs that you will be powerless to fix until the library itself is patched up. Furthermore, if you use a library that "simplifies" things, you will be forced to learn a new pattern of building the thing in question. If the library ever gets abandoned or deprecated (which it will), you will need to re-learn the original pattern of building the thing. Therefore, unless a library is extremely popular (e.g. Butterknife, Retrofit, etc), or is poised to become part of the framework itself, I'm generally allergic to it. But that's up to you.
• I'm especially wary of using a custom library for a single specific purpose in the product. For example, you're using the com.borjabravo:readmoretextview library for a single TextView in a single spot in the app. Is it really necessary? My feeling is that the decision to import a new library should come from a strong overall pressure from many facets within the product, rather than a spur-of-the-moment choice made in a random pull request.

Other minor points:

• It looks like the app is using Dagger to inject dependencies in various locations. The side effect of this is that the code for initializing certain components (Fragments/ContentProviders/etc) no longer looks "standard," and appears more complex than it needs to be. I'll admit that I'm not fully familiar with Dagger and its benefits, but I'll ask this question regardless: has Dagger helped you significantly? i.e. how many bugs has it helped to identify and fix? What I'm suggesting is that integrating dependency injection before the code itself is brought up to modern standards may be putting the cart before the horse (because now you'll need to refactor the dependency injection logic along with the code itself).

• The app seems to be using at least three (or four?) separate SharedPreferences databases. Was there a rationale for this, and is there a reason we can't consolidate them into one?

• There's a custom truetype font that is bundled in the assets (Ubuntu-R.ttf). How/where is this used?

• There are still a lot of string literals throughout the code. I would recommend sweeping through all of them and converting them to string resources. I know it's tempting to throw in a string literal in a hurry, but investing a little more time to create a resource is very much worth it, since we get a free translation service (TranslateWiki) which is very costly for other apps.

[nicolas-raoul]

Getting rid of legacy HTTP APIs would be a wonderful thing! If we are lucky that might even solve #1866? :-)

[misaochan]

Thanks again for the detailed analysis @dbrant . :)

Now that some of the excess complexity will be removed by #1859, I would strongly recommend prioritizing the elimination of the woefully outdated mediawiki:api library and replacing it with Retrofit, using the json API, coupled with RxJava (This will also remove the need for the outdated yuvi:http.fluent and org.apache.http.legacy libraries if I'm not mistaken). I see you have an open pull request where a volunteer takes some first steps towards this refactor, but the volunteer seems to have dropped off. If you like, I could take it the rest of the way, as part of my 10% time at work. ;)

This would be wonderful if possible, thank you! The mediawiki:api and org.apache.http librares have been the cause of quite a few bugs indeed.

I agree with most of the other points, too. However, while I was initially dubious about Dagger, when I got around to using it I personally did feel like it helped simplify the creation of a shared model, rather than using SharedPreferences to pass data around. But it is true that Dagger did cause its own share of problems, and in any case we should certainly have prioritized the overhaul of legacy code first.

Based on this discussion, I would like to propose an answer to my own question "What sort of guidelines should we use to determine whether a new library should be added or not, in the future?":

• Nobody should be able to just include a new library in a PR. New libraries can only be added if an issue is created specifically for including the new library, and the person proposing it has good reasons for why the library would benefit the app as a whole, rather than just making it easier for them to do that one specific thing that they wanted to do in their PR. At least 3 other collaborators must agree before the inclusion can proceed. (We can change this number as activity fluctuates)
• On a tangential PR note: We should be more strict about string literals in PRs. Please don't merge any new PRs with string literals.

The main concern with these guidelines is that it will take a bit more work, and also take a longer time, for PRs by volunteers to be merged. Currently we already do take a fairly long time to merge volunteer PRs, because only one of us (@neslihanturan ) is tasked with testing and reviewing PRs as part of their job role, and she has other tasks to handle as well. But I think that the slight additional wait will be more than worth the improvement to our codebase.

[sivaraam]

But I think that the slight additional wait will be more than worth the improvement to our codebase.

May be we the volunteers should be made known about the limited amount of resources to review their PRs in some place? Possibly in the CONTRIBUTING.md file?

This might help them realised the reason for the delay (if they haven't figured it out correctly.

[sivaraam]

Based on this discussion, I would like to propose an answer to my own question "What sort of guidelines should we use to determine whether a new library should be added or not, in the future?":

Now that you have an answer, it might worth documenting it somewhere. Not sure where. Maybe in a Coding guidelines document?

[misaochan]

Done, see https://github.com/commons-app/apps-android-commons/wiki/Code-style#new-libraries and https://github.com/commons-app/apps-android-commons/wiki/Project-maintenance

[dbrant]

Here is the current plan from our end:
Within the next few weeks, in the Wikipedia app we will work on isolating our network layer from the rest of the app, so that we could package it as a standalone library, basically an official successor to the mediawiki:api library, to be usable by your app as well as ours.

It will provide a superset of the API calls used by our respective apps, along with numerous convenience functions (e.g. formatting dates, GeoIP, etc), which will minimize code duplication between our products. It will also use all the latest best practices, including Retrofit, and outputting Observable objects.

Once this is done, I will contribute towards reworking the Commons app code to make use of the new library, which will hopefully trim away a lot of duplicate code, and increase both of our teams' speed and confidence when adding new features.

[misaochan]

That would be fantastic! Looking forward to it. :)

[maskaravivek]

Really excited about it. :)