Code improvement ideas

[tabasavr]

I've been contributing to Infinity for some time now and I came up with some ideas that I believe can improve the overall quality of the code. Since they aren't actually fixing any bugs and some ideas require massive refactorings, I wanted to get some feedback first. Please let me know what you think about this, if you agree/disagree with the ideas or if you want to discuss them in more detail.

Here's the list. There is no strict order but I tried to put simpler stuff first and more complex stuff at the end.

• Fix lint issues. 2600 errors/warnings. 1700 of them are NonConstantResourceId about layout ids used in ButterKnife. Some are real things worth fixing, others can be added to baseline/ignored.

• Fix android studio warnings. 4000 warnings, 1000 of them in Android Lint category. Also 1000 of them are about making fields final. 200 are deprecation warnings.

• Run lint and static analyzer as github actions.

• Use IntDefs and StringDefs. Examples: vote type, placeholder type. Makes code more self-documenting without switching to enums. Will help catch errors like forgotten comparison or wrong constant.

• Use ColorInt, StyleRes and other annotations. Same benefits as previous one.

• Use nullability annotations. There are some personal preferences (does everything need to be annotated? are non-annotated fields nullable by default?) and not everything can be annotated. But combined with final fields this might simplify some code.

• Fix LiveData usages. There are a couple of places similar to Transformations.switchMap(someLiveData, someValue -> doStuff(someLiveData.getValue()));

• Reduce boilerplate for accessing SharedPreferences. There is a lot of code like (accountName == null ? "" : accountName) + SharedPreferencesUtils.SOME_KEY or parsing a string from preferences into int. It would be nice to abstract such details. Maybe also migrate to DataStore library, but that's probably not worth it.

• Reduce code duplication. This is a part of many ideas, but sometimes it is just duplicated code. Examples: ItemSwipeHelper, voting in PostRecyclerViewAdapter, adapter callbacks.

• Reduce the number of Retrofit api creations. This can be done by injecting Provider<API> instead of Retrofit instances. Has performance benefits and uses types to catch errors compile time.

• Use Retrofit converters and optional query parameters for sort type and time. Allows to call api.foo(sort.getType(),sort.getTime()) without null checks. Or maybe just optional query and helper methods: api.foo(sort.getTypeValue(), sort.getTimeValue()). I actually prefer the first variant because it does not allow any arbitrary string as parameter.

• Replace ButterKnife with ViewBinding. This is needed anyways because of changes in Gradle 8. Included in lint issues.

• Update paging code. Included in android studio warnings. Also handle loading status by the paging library.

• Apply auth headers using Authenticator/Interceptor. This should simplify logic and remove code duplication. Should be careful with account switching, but current authenticator implementation doesn’t really handle account switching anyways. Also related to this is an option to create OauthRedditAPI and NoOauthRedditAPI, both extending RedditAPI and inject one of them. This allows to add type safety without duplicating common paths.

• Stop updating data based on getBindingAdapterPosition() and use its id instead. Because ViewHolders can be reused, this can lead to bugs: by the time callback is called ViewHolder can be bound to another piece of data, so wrong data will be updated (e.g. Pressing "See removed comment" will "edit" wrong message  #822).

• Stop updating views in ViewHolders manually and use notifyItemChanged, maybe with payload argument for efficiency. Reduces code duplication.

• Use Dagger more. This one is tricky, it looks like there is no more code that could be injected with dagger right away. But the two potential use cases are Fetch* classes and ViewModel factories.

• Use constructor injection in fragments. Available in new versions of androidx.fragment library by using a factory. Allows to make more stuff final/immutable.

• Treat more ui settings changes like configuration changes and recreate activities (or just adapters). Allows to make more stuff final/immutable and simplify code.

• Separate adapters and data manipulation. Simplifies code in the sense of following single responsibility principle and separating unrelated things.

• Move to a “single source of truth” architecture. Related to the previous one. Enforces consistency of data everywhere by allowing changes only in one place.

[tabasavr]

@Docile-Alligator what do you think about this?

[Docile-Alligator]

Thank you for these!

Fix LiveData usages. There are a couple of places similar to Transformations.switchMap(someLiveData, someValue -> doStuff(someLiveData.getValue()));
Could you elaborate more about this?

Update paging code. Included in android studio warnings. Also handle loading status by the paging library.
Do you mean updating from paging 2 to paging 3?

Stop updating data based on getBindingAdapterPosition() and use its id instead.
Use whose id?

Use Dagger more. This one is tricky, it looks like there is no more code that could be injected with dagger right away. But the two potential use cases are Fetch* classes and ViewModel factories.
Why do we need to inject Fetch* classes and ViewModel factories?

[tabasavr]

Fix LiveData usages. There are a couple of places similar to Transformations.switchMap(someLiveData, someValue -> doStuff(someLiveData.getValue())); Could you elaborate more about this?

switchMap already provides liveData's value, there is no need to call getValue again. So it should be rewritten as Transformations.switchMap(someLiveData, someValue -> doStuff(someValue));

Update paging code. Included in android studio warnings. Also handle loading status by the paging library. Do you mean updating from paging 2 to paging 3?

Yes

Stop updating data based on getBindingAdapterPosition() and use its id instead. Use whose id?

This is mostly about comments because their structure is very dynamic - collapsing/expanding, loading more, deleting. All such changes change position of the comments. Because of this retrieving a comment by its position in a callback can return wrong comment which leads to bugs.

Also in general viewholders can get bound to new items, so accessing viewholder's position in a callback is error-prone.

Use Dagger more. This one is tricky, it looks like there is no more code that could be injected with dagger right away. But the two potential use cases are Fetch* classes and ViewModel factories. Why do we need to inject Fetch* classes and ViewModel factories?

I think there are two ways of interpreting your question:

1. "Why use dependency injection". There are a lot of great articles on why you should and shouldn't use dependency injection and DI frameworks, so I'll skip this one.
2. "Why change Fetch* classes". Right now they are just containers for static functions and for every method call you have to provide retrofit, executor, database, preferences or whatever else the method needs. Basically you are leaking implementation details that the caller shouldn't care about. Instead of that I propose to change them into actual classes that would get constructed with everything they need to work (retrofit, etc.) and their method would require only actually changing arguments like ids.

[tabasavr]

Because of this retrieving a comment by its position in a callback can return wrong comment which leads to bugs.

Also in general viewholders can get bound to new items, so accessing viewholder's position in a callback is error-prone.

To clarify this a bit more, it is OK to use position in click listeners and other places that need to access the data that is currently displayed by the view holder. The problem is with callbacks from, for example, network requests because there is some delay before they are executed. By the time they are executed vieholder might be bound to a different data than the one originally used to make the request. Moreover, the position of the vieholder desn't have to change - theoretically collapsing some comments and expanding other can result in a view holder that is bound to the same position but a different comment!

I guess the conclusion/general advice is to call getBindingAdapterPosition only once at the beginning of the interaction, get the data based on that position and then either work directly with that object or find it again using a different method, not dependent on vieholder's position.

[tabasavr]

I don't have much free time anymore, so I'm going to stop actively contributing to Infinity. However there are some ideas that I think are cool but I haven't completed implementing. Feel free to use them however you want.

• Lint as GitHub Action
  Needs an updated baseline and something was crashing/not working

• Set auth headers in an interceptor
  Needs some benchmarking to check if reading from DB on every request is ok. Alternative is to use accessToken instead of accountName. However using accountName has one very important advantage - requests and re-requests are guaranteed to use the same account. Current implementation ignores account changes and changing account between request and execution of authenticate results in a re-request with access token of wrong account.

• CustomThemeInflater
  An inflater that inflates views with colors from CustomThemeWrapper already applied. Can significantly reduce the amount of code required for theming and allow almost painless support for dynamic colors. Implementation is a bit complicated (single color ColorStateList with reference to theme attributes) but it allows both to use colors defined in theme and compare ints instead of strings during inflation. Also needs benchmarking. And I'm not happy with how AppComponent is accessed, static field in Infinity probably would be better.

• System language picker
  Support for Per-app language preferences. Was waiting for appcompat:1.6.0 stable release and needs some fixes.

• An attempt at refactoring CommentsRecyclerViewAdapter
  Has a lot of different stuff: separating updates from adapter, single source of truth, search by fullname, notifyItemChanged everywhere, some other minor stuff. Despite its huge size it is still far from finished, e.g. there is no ViewModel.

• Apply menu icon disabled alpha via ColorStateList
  Using ColorStateList exactly for what it was designed. Should need just some cleaning up