Code Review 1 - AppArt

[zyuiop]

This is a review of the entire codebase at commit cd0d412.

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.

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 some of the issues below.

General Remarks

• We didn't succeed to read the app. There is a NullPointerException thrown by Card, at line 36 (the null check). Maybe the data on Firebase is incomplete? But you should probably handle this more gracefully than a crash.
• A few classes lack documentation, in particular exceptions. Their names are often self-explanatory, but some additional details can still be useful.
• Overall, your code structure and architecture are very well thought and were a pleasure to go through and review. We therefore don't have a lot of remarks to make on your code, and most of them are nitpicks. Good job!

By File

Package ad

• (Nitpick) In ContactInfoDialogFragment, you use LiveData#getValue to set the text of the fields, we think using observe would be slightly cleaner (e.g. do mViewModel.getEmailAddress().observe(this, email::setText);)

Package adcreation

• (Nit) We're not 100% sure why you used LiveDatas in this viewmodel instead of simply multable fields, since you don't seem to observe on these livedatas anywhere.

Package database

• No documentation in the exception exceptions.DatabaseServiceException: in which case is this exception fired? What should developers do to handle it.
• The documentation in the package is otherwise very nice and complete, kudos!
• The database code is very clean, despite the minor remarks below.
• You do Serialization/Deserialization of data directly in your FirebaseDatabaseService. We think it may be interesting to define this elsewhere, so that you can test your serialization process in isolation from Firebase. You could for example define a generic interface Serializer and Deserializer, with methods to serialize from/to a Map<String,Object>, and provide an implementation for each datatype which you could then inject using Hilt in relevant places (i.e. FirestoreDatabaseService).
• You have all your data managed through a single DatabaseService interface. If you start managing more data-types (chat messages, visit requests, ...), you may want to have an interface for each datatype (i.e. CardsDatabaseService, ProfilesDatabaseService, etc.)

Package glide

• (Nitpick) Naming: DatabaseHostVisitor should probably be named DatabaseHostVisitable

Package login

• Very nice architecture!

Package scrolling

• The Card dataclass could (and should probably) be immutable. If you need setters, you can make them return a modified copy.
• In ScrollingViewModel you don't handle Database errors

Package user

• Why use an interface for User? It seems like it could be a simple dataclass.
• Gender: the COUNT field is useless. Use Gender.values().length. It's also unused.
• (Nitpick) You could implement these two filters as predicates (interface java.util.Predicate<T>), they're basically an abstraction for functions that take an argument and return a boolean (implement test), and provide some composition utils.

Tests

Some of your tests don't assert anything. Please remove such tests or add assertions.

Examples:
- All tests in CameraUITest
- CreateUserAccountUITest.failedCreateAccountTest
- ...

Conclusion

Your code is very well structured and clean. We really liked the way you abstracted the database and login features behind interfaces. Keep un the very good work!

[rovati]

Thank you very much for the review and the feedback, we're glad you are satisfied with the work done so far!

Regarding the app crashing, we realized just after merging the refactoring PR that we forgot to adapt the data in firestore to the new naming conventions we started using locally. We already have opened a task to solve this issue and we will let you know as soon as it is completed so that if you want to take a tour before Friday's demo you will be able to do so.

We also are aware of the fact that we are not dealing with database errors. We've been trying to discuss about this for a couple of weeks already, we hope we can settle on the design anytime soon so that we can work on it in the next sprints.

Thanks again for the tips and see you Friday!

[rovati]

Regarding the app crashing, we realized just after merging the refactoring PR that we forgot to adapt the data in firestore to the new naming conventions we started using locally. We already have opened a task to solve this issue and we will let you know as soon as it is completed so that if you want to take a tour before Friday's demo you will be able to do so.

The Firestore data has been modified, now you should be able to successfully run the app! The UI has minor layout issues that are being fixed in this sprint.