Code Review - Multimatum

[ros-dorian]

Code review - Multimatum

This is a code review for the code of the whole project on the main branch at commit https://github.com/multimatum-Team/multimatum/tree/522371a534f13f99bbf2f41c90b93ddda906aaa8

Of course, we don't expect you to change all of your code (some parts of the review can be subjective or arguable anyway), what’s important is that you understand why better alternatives may exist.

Please ask us if you have any question about the code review, whether it is by writing a comment below or by sending us an email or meeting us.

Important : if you try to fix these, don't break your app. We prefer that the app works even if you don't manage to fix anything. It may be too hard to rework many of the issues below, as it would require to rewrite the whole architecture.

General remarks

Package structure

Overall, your package structure makes sense and clearly separates the different modules in your app. Some classes seem to be out of place (LogUtil.kt, ProfileActivity.kt), but maybe you have a good reason to do this.

One thing however, in the repository package you could create a sub-package firebase to store all of your Firebase related implementations.

Also, maybe it would make more sense to put the datetime_parser package as well as QRCodeGenerator.kt inside of the util package.

Finally, you seem to have misspelled adapter :)

Documentation

Good job on the documentation. Almost all public interfaces have some sort of comment indicating its function.

Code style and quality

Your code is readable and well formatted. It also seems to be quite consistent across all files in the project. Keep it up and don't forget to run and address the linter warnings that Android Studio can provide you.

Good job on your Repository/ViewModel structure. You make good use of coroutines and the code is very modular.

Regarding the UI, some activities could be turned into fragments (e.g, MainActivity + CalendarActivity). This would let you share the viewmodel which removes some overhead when initializing it.

Classes

ReminderBroadcastReceiver.kt

It could make sense to store string identifiers such as “id”, “title”, etc… in static const variables to avoid confusion when using them outside of this class.

DeadlineListViewModel.kt

getDeadline could probably also return a LiveData for consistency.

The constructor uses a lot of runBlocking. Maybe you could find a way to make it asynchronous as it would make the app more responsive.

DeadlineNotification.kt

This could perhaps be made into a viewmodel or integrate it into the DeadlineListViewModel. The latter would make sense as it is currently used whenever interacting with the deadline viewmodel.

PDFUtil.kt

This does not need to be separated into a new class as it is only used in one place. You can instead create a function in the activity in which it is used.

DeadlineAdapter.kt

It could be wise to remove the DeadlineListViewModel dependency and replace it with a simple callback that you can inject in the constructor (something like onDeadlineChecked). It would make it easier to test and it would remove a pretty big dependency that is tied to the lifecycle of the application.

UserGroupAdapter.kt

Same thing as in the previous comment. Maybe it would be better not to have the GroupViewModel dependency here. However, you may have a good reason to need it ?

MainActivity.kt

You have a dependency for DeadlineRepository which is not used.

You call Firebase.initialize(this) which creates an unwanted dependency with Firebase. Could this be placed somewhere else ? (e.g., in the viewmodel ?)

SignInActivity.kt

This activity does not make use of a repository/viewmodel which is a bit odd. This creates a tight dependency with Firebase.

Testing

You should try to follow the same package structure as in the source code. This makes it clearer where each test class is with respect to the class that is being tested.

Your test functions should follow the same naming convention as the rest of the codebase (camelCase for Java/Kotlin or the string representation like in AddDeadlineTest.kt).

GroupActivityTest.kt seems to be empty.
All tests that need an Android context (activities, services, etc…) should be placed in the androidTest package.
The ProfileActivity seems to not have tests. This could be a straightforward way to increase coverage.

Conclusion

Overall, this is a very nice codebase. You have a well defined architecture that should be easy to extend and is easy to follow.

Great job!