Interim Code Review

[rishabh246]

Hi,
This is a code review to help you improve your codebase.
To be clear, this review will not affect your grade by itself, but at the end of the class part of your grade will be on the code, so this review should help you improve the code sooner rather than later.

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 : Its not necessary that you fix all of these. However, if you do try, don't break your app. We prefer that the app works even if you don't manage to fix anything.

The below review was done after the commit a7ec9b2110b0f18ad208568f3878a656a6dd8196

Overall, the code is well organised and stable, but there is scope for improvement.
Here is feedback on specific aspects of the codebase:

• Codebase Architecture: The top level of your current hierarchy is well defined and intuitive. However, there are few issues:
  • The notifications package is architected poorly. Currently, the notification interface is within the ConcertSoon package and is imported by the ConcertFlow package. You should have a common interface across all notification types.
  • Classes LocationUtil and POIUtils are in their specific packages, while all other Util classes are in the Util package
  • Certain package related activities (e.g.. Settings, MapView, Social) are inside their respective packages, while others are in the main package. Fragments (so far) are all within their respective packages. This should be more uniform. One way to do this is to retain only top-level activities in the main package and keep sub-package specific activities and fragments within their own packages.
  • Nomenclature: Your nomenclature is often confusing, here are two examples:
    - Your organization has two such packages: balelecbud.emergency.models and balelecbud.models.emergency. The difference is unclear.
    - The subpackage in Transport is called objects, while for Emergency and FestivalInfo, its called models. As far as I can tell, their purpose is identical. If so, please remove the inconsistency.
• Documentation: Your codebase has no documentation whatsoever. This needs to be fixed. We do not need you to document every method (this often leads to bloated code), but you should at least document each class and preferably, every public method of the class. Further, documentation can also help improve your package architecture. One way to ensure packages have a meaning is to create a package-info.java file for each package, filling in the Javadoc; this will force you to write down what packages are about and thus to think about it.
• Layout: Its good that you break your layout into activity layouts and resources. However, there is still quite a lot of redundancy (and hardcoding) in the resources. For instance, every item has identical padding at the top and bottom, yet this is repeated as a constant in every resource file.

Some nits:

• I am a little confused by your package ch.epfl.balelecbud.models. I don't see why you cannot simply have these classes (e.g., location, Emergency) inside their specific packages?
• Classes EmergencyInfo and EmergencyNumber are the same and should be merged.