Building Better Android Apps One Code Review at a Time

Enhance Your App’s Quality and Maintainability

Everyone thinks their ending result of programming is the best, however, that is only sometimes the case.

We are still human and we make mistakes. Another pair of eyes can reduce errors and eliminate the accumulation of technical debt and incorrect practices.

Every development team is different and they should do the work as it works for them. Do not enforce anything that is causing you more trouble than benefit or practice which exists just for its existence.

This article will describe Android-specific tips, but If you are interested in more general approach, you can read about it in this article.

When I make the review, I go from high-level domain to low-level items, so let’s start.

1. Architecture

Random 3rd party packages

It is tempting to use random 3rd party packages to solve all the problems, but they can pose more problems than usual benefits.

You impose limitations of the package on your current implementation like deprecated implementations, heaviness of solution, duplication of code, maintenance and licensing issues. Sometimes, the package can be a source of vulnerabilities and potentially malicious behaviour.

For example:

• most of the UI components can be recreated with the use of already provided UI Views/Composables without the need to plague the project with another package
• HTTP calls, storage and databases should use one library at most - look for already existing API or popular libraries and do not mix them redundantly
• many libraries provide multiple same functionalities and dropping one in favour of another can increase the robustness and size of the final app

In many cases, it is worth it to implement it on your own. You have full control over the functionality and full ownership.

Before usage, look at credibility of the package, if it is actually used by people, actively maintained and if the license is permissible for your usage.

Coding patterns

A project usually has its philosophy in terms of architecture and the main pattern used. It can be MVVM/MVI/MVP and others…, and it should be followed across the project.

Every architecture imposes certain rules about which layer is responsible for what. Usually, UI, business logic and repositories are separated. In the case of mixing the layers, code change should be required.

Here are a couple of examples:

• UI elements should not be bothered by the extensive logic
• Business logic/use cases - should not be responsible for taking care of UI elements
• native API calls - should be separated of the business logic to preserve testability
• LifeCycle events - ViewModels and other business components should be able to clear out their memory to avoid memory leaks

Good rule of thumb is, if you are able to test your logic in unit tests with mocking of native repositories only, then you achieved good level of separation.

2. Functionality

If the architecture seems right, we can go level further and inspect the actual implementation of the feature.

We should always check if the code does, what is desired from the requirements.

Asking for demo is sometimes the simplest thing, what you can do, because code can tell one thing, but actual behaviour could differ from the expectation.

UI

The final UI should match the proposed design and changes should be allowed only after discussing the proposed change. Sometimes it is better to change the design a tiny bit to make the implementation much more straightforward.

Check:

• dark mode - if supported
• landscape mode and transitions - if supported
• running the UI on small/large devices or tablet
• accessibility via font size enlarging

Error handling

To achieve a happy path to work is one part of success. Useful error handling in another part of the code, which should be robust.

Check for handling of:

• unexpected inputs from the user
• known errors which can happen during the process
• error messages, if they are helpful
• offline mode - user turns off wifi/cellular connection
• lifecycle change - user turns off the screen for a couple of minutes
• missing permission - user/Android revokes the permission after some time
• process death - low memory causes the killing of the app

Try to prevent:

• leaking private information of the user through logs/stack traces
• using many different error types, which could be passed as app-specific error
• over-engineering error handling - sometimes it is better to let the process fail and catch the error on the top layer of business logic

3. Complexity

Let’s assume the functionality now works and handles everything that is required. However, this does not mean that it is correct.

Code related to new features should be readable

If you can read through the implemented feature and it is clear, what has been implemented, then it is a good thing. However, if you struggle to comprehend, what is going on, then it is a good candidate for improvement.

The feature might be too fragmented or use too many dependencies.

A couple of tips:

• concentrate the feature based on the given architecture and split it appropriately - use dependency injection to provide only what is needed
• too many dependencies can be a sign of over-engineering and can be prevented by splitting the responsibilities into use cases
• looking at the folder/module structure, if it can be simplified or more organized - e.g. every feature/screen to have its folder. Shared features should have their module.
• avoid extensive nesting at any place - avoid multiple layers of UI at one place or using multiple if statements inserted into each other. Use a more sequential approach. Separate UI into named composables or use multiple if statements in sequence.

Simplicity is over complexity. Showing easier way is learning opportunity.

Is it speculation or a solution?

We try to prevent our app from failing/slowing down and that results in sometimes adding functionalities for “just in case” reasons, which are for the most part redundant.

• eliminate dead code or generalize it in a way, so you do not have to think about it - e.g. over-using try/catch. Catch errors at a certain level and let others happen.
• don’t optimize the solution, if it is not needed
• however, the proper context and scope for the coroutine task should be always used
  • use the ViewModel scope to launch ViewModel operations
  • Dispatchers.Main for all UI updates
  • Dispatchers.IO for the loading of files, database and API calls
  • Dispatchers.Default for parsing and processing large amounts of data

With higher complexity, you can achieve some advantages, but they are often minimal in comparison to future maintenance and speed of the code.

Opting for tinderbox solution is sometimes preferable instead of using flamethrower.

Monitor Performance Only When Necessary

You will always find a place for performance improvement. You can always squeeze a couple of milliseconds from the algorithm. However, do not optimize if no one complains. You might be wasting time, which should be invested somewhere. where is the actual value.

Good rules of thumb:

• usage of proper coroutines dispatchers
• updates UI, only when it is needed
• uses appropriate lazy variations of lists and loading - load only what is needed, not everything at one time

4. Testing

Tests are part of development, which can save you more time in the future than you invested in them. Do not treat tests as second-class citizens in your codebase.

Check if:

• tests provide reasonable coverage and go through all common scenarios
• test doubles (mocks) are used reasonably and do not hide the issue
• code test actual changes
• irrelevant tests have been removed
• valid mocking and testing frameworks are used
• tests are consistent with the rest of the testing suite
• factories are provided for new classes

Here are a couple of articles related to testing:

• Too many dependencies and factories
• Test tags and semantics
• Robot pattern and compose testing

5. Code styling

Last, but not least is the actual code styling. You can have beautifully styled code, but if the architecture is not good, you will end up rewriting it anyway.

Even tests are above, because, if you are not able to write tests for your implementation, then there is something wrong with it.

What to look for:

• Explicit and Intentional Naming of Variables: Instead of i, use index or currentItemIndex to indicate the purpose of the variable in a loop.
• Naming of parameters in functions: proper naming of parameters can make clear for what they are for. Avoid using generic names, if you can be more specific. E.g. delayInMillis: Long - We know it is delayed and it is in milliseconds just from the name.
• usage of @StringRes or @ColorInt
• preserving Context at any point: Instead of keeping the context, do the required job at the moment and then release it to avoid memory leaks
• Comments Explaining More Why This Code is Here Than What it is Doing: if something unexpected / not obvious is needed, comment it why it is needed, not for what it is needed
• Applied Styling by IDE: Use your IDE’s formatting options to automatically indent code blocks and add consistent spacing.
• FIXME and TODO Comments: Add a FIXME comment to mark a potential issue that needs to be addressed later, or a TODO comment to track a task that needs to be implemented.
• Description of Tests and Their Arrangement - proper naming can make clear, why this test exists like: testButtonClickOpensNewActivity
• Redundant Changes in Code with No Benefit
• Fits the Design of the Project: If your project uses certain architecture, ensure that your code adheres to the separation of concerns

All linter findings should be addressed at the end of code changes.

Code of behaviour:

• be polite
• be helpful
• be reasonable
• leave actionable feedback
• guide junior developers, so they can grow to your level
• technical fact overrules the opinion
• invite other developers to share knowledge or make them familiar with large changes
• do reviews promptly

Conclusion

Core review is the point of development when we decide if the changes are valid for the project. If you agree, you are responsible for them too and take time to review the code properly. At some point, you might be the one fixing the mess, which you agreed to that is fine.

Hopefully, these tips will make it easier for you to go through the code review and improve the codebase of your team.

Thanks for reading and do not forget to follow!