# H3LP Code review
# General Remark
Of course, we don't expect you to change all of your code (some parts of the review can be subjective), 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.

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.
## Code structure
The package structure is well organized. However, it should be nice to split the view and the model in different packages.

Several times, you define `goToActivity` functions in different activities. Maybe you could investigate if there is a better way of doing this.

Otherwise, you seem to use a large amount of Kotlin functionnalities which is a really good thing.
## Code style
*Globally, your code style is consistent. In some files however we saw some variations: single line comments had no space (i.e. you did `//comment` instead of `// comment`), no spaces around a `==`... We suggest you use the automatic code reformatting from Android Studio before commiting code.*
More generally, some activities are not commented and others are, you should really comment every activity with their goals.
# By file
*Only files for which we have a comment are referenced here.*
## Package `h3lp`
### `AwaitHelpActivity`
- `setupLocation` documentation refers to an invalid method.
- See comment on `FirstAidActivity` for the functions at the end of the class.
- Missing comments on some functions (`foundHelperPerson`, `showHelperPerson`)
### `FirstAidActivity`
- We propose to refactor the activity in the following way:
- Create a map, mapping the button id to an intent going to the Activity it refers to.
- Having a single function goToActivity(v:View) that uses `v.id` to launch the correct activity.
- this way, we avoid the code duplication you have at the moment.
### `GoogleAPIHelper`
- `downloadURL` missing @return comment.
- (`displayWalkingPath`, line 51) We detect some inconsistency here. Refers to the comment on `JSONParserInterface`. We encourage you to return `null` value from the JSON parser if an error happen. To handle the `null` value, instead of `path.let` you can use `path?.let` and in the function you can use `it` instead of `path`.
- `parseTask` missing @return and @param comments and return type should be changed to `T?`.
### `HelpParameterActivity`

### `LocalEmergencyCaller`
- `getPhoneNumberFromCountry` the reader is not closed properly `csvReader.close()`.
### `MainPageActivity`
- See comment on `FirstAidActivity` for the functions at the end of the class.
- Missing comments on the `MainPageButton`.
### MapsFragment
- `showMap`: since the hashmap place is a `HashMap<String, String>`, the returned value cannot be `null` and therefore the two last conditions on the first if are not needed.
### `MedicalCardActivity`
- `createBirthField`, `createHeightField`, `createWeightField`, `createBloodField`, `createGenderField`, `createHelpField`: the content of these methods could be directly replaced in the onCreate() function (keep their comments though) since they are called once and contains only one line on code.
- The two `loadTo` methods don't have comments.
- Missing @return comment for `checkField` and `checkPolicy` functions.
- Missing comments for `getStringFromId` and `getIntFromId`

### `MySkillsActivity`
- `getBooleanFromSwitch`, `toggleSwitch` missing comments.
- Remove the annotation after having upgraded the SDK version

### `NearbyUtilitiesActivity`
- Missing comments `setRequestedButton` and `setupLocation`
## Package `h3lp.database`
### `EmergencyInfoRepository`
- @param for `database` is missing.
### `Repositories`
- It might be better to wrap everything in an `object` instea of having "global variable".
### `Database`
- The signature of the `addListener method should change (according to the comments we will make in the `FireDatabase` class).
### `Databases`
- @return missing for the return type of databaseOf method.
### `FireDatabase`
- In the JavaDoc comment of the get method, should have Long instead of Int. Missing comment for the returned value.
- The comments of the overidden functions should not be rewritten in sub-classes/interface implementation.
- Missing description for the `path` parameter of the class.
- To be consistent with the `get` function, it would be nice to also the `reified` keyword instead of explicitly asking the type as a parameter of the function, since the class.java can be obtained from the `T` type (using `T::class.java`).
- In the same function, for the `onCancelled` case, you should write the error in the log (using `Log.e(...)`) instead of printing a message in the console.
-
### `MockDatabase`
- This class should be in the test package as it is not used during the normal runtime of the application.
- The method `addListener` will change according to the comments made above.
## Package `h3lp.dataclasses`
### `HelperSkills`
- No comments for the different values of the data class.
### `MedicalInformation`
- No comment for the different values of the data class.
- You should upgrade the API version of your app instead of having `RequiresAPI(...)` annotation.
- The `MIN_WEIGHT` cannot be set to 20 if the birthdate can be today. However, for legal purposes (ask us in the next meeting for more information), it should be preferred to limit the age of the app user.
- Small typo in the `BloodType` enum, `Unkown` should be replaced by `Unknown`.
## Package `h3lp.firstaid`
- Instead of having 4 different activities with the same structure/same code and different content, you could have only one activity which takes a parameter [(more info)](https://stackoverflow.com/questions/2405120/how-to-start-an-intent-by-passing-some-parameters-to-it) and adapt the content depending on the parameter. This avoid code duplication and is better practice.
## Package `h3lp.locationmanager`
### `GeneralLocationManager`
- The way you did the get/set property is fine. However, for more concise notation, see [properties documentation](https://kotlinlang.org/docs/properties.html#getters-and-setters) of Kotlin.
- Minor issue but it could be nice to change the name of the last function from `setSystemManager` to `setDefaultSystemManager`.
### `LocationManagerInterface`
- Missing comment for return value
### `SystemLocationAdapter`
- No need to comment the function again since it is an overidden function and the comment is the same.
## Package `h3lp.notification`
### `NotificationService`
- `createNotificationChannel`, you can upgrade the SDK version of Android to avoid this if.
- In `sendOpenActivityNotification`, you can use `reified` keyword instead of explicitly giving the type as parameter.
- `buildBasicNotification`, minor typos in the @return of the function.
## Package `h3lp.presentation`
### `PresIrrelevantActivity`
- One improvment could be to change the color of the button when we agree to the terms of use.
- Missing comment for `addClickableText`
### `SwipeListener`
- Missing comments in the companion object.
## Package `h3lp.signin`
### `GoogleSignInAdapter`
- No need to comment again for overidden functions.
### `SignIn`
- You can use properties as suggested before. If you don't want to, you should comment the set function.
### `SignInActivity`
- As said in the general remark, storing context is a bad practice [(see this)](https://stackoverflow.com/questions/37709918/warning-do-not-place-android-context-classes-in-static-fields-this-is-a-memory). Therefore, the context should be removed from the companion object.
- Missing comment for the functions `checkToSAndLaunchIfNotAcceptedElseMain`
- If `signInClient` is not used elsewhere, should be private, otherwise you should comment what it represents.
### `SignInInterface`
- Really nice abstraction
- Missing @return comment for `isSignedIn`.
## Package `h3lp.storage`
### `LocalStorage`
- `pull` should not throw a `NullPointerException`. You should throw a more explicit Error (e.g. `UserNotAuthenticatedException`) that will be catched later on.
- Same remark as earlier for the `context` object stored in the instance of the class.
- Missing @param comment for `parseOnlinePrefs`.
- Extract one function containing the db object creation, that will throw a more meaningful error if the user is not logged in.
- The comments (@param, @return) are missing in this class for almost all the methods :).
- These methods need to be implemented asynchronously since disk I/O are slow operation that should not be blocking and to be consistent with what you did for the online database.
- `getObjectOrDefault` and `setObject` should use `reified` keyword instead of explicit class type.
### `Storages`
- Same remark as earlier for the `context` object stored in the instance of the class.
- `storageOf` missing @return comment.
- Missing comment for class parameter.
## Package `h3lp.util`
### `AedLocations`
- The value `AED_LOCATIONS_LAUSANNE` should be wrapped in an object. Also, the aed locations should be store in the online database and retrieve when the application needs it and eventually cached locally (since the location is unlikely to be changed too often).
### `GDurationJSONParser`

### `JSONParserInterface`
- `parseResult` should return `T?` instead of `T` otherwise you have no way of knowing if there was an error during the parsing.
# Tests
Your tests look very good, congratulations for that.
# Conclusion
Your code is very good overall, you have nice abstractions and lots of efforts put in. The documentation and maning are mostly clear and understable, the codestyle is mostly consistent. The app design is very cool. Keep up the good work!