Skip to content
This repository has been archived by the owner on Jun 22, 2022. It is now read-only.

Code review #137

Open
jovankomatovic opened this issue Apr 30, 2020 · 1 comment
Open

Code review #137

jovankomatovic opened this issue Apr 30, 2020 · 1 comment
Assignees

Comments

@jovankomatovic
Copy link

Overall
Your code is in a stable state.
However, there are rooms for improvement.
Architecture of the code can be improved (in particular, packages and classes that belong to each package should be revised).

Code

  • You are inconsistent with the use of braces in conditionals when they are not strictly needed (one-line "then"). Please, be consistent with respect to this. We suggest to use them always (even when they are not needed) since they help with clarity of the code.
  • Define constants in app resources (avoid raw values in the code).
  • The map package should be reconstructed entirely. SearchAreaBuilder, PolygonBuilder and QuadrilateralBuilder represent the separate entity (and you have searchareabuilder package). The same holds for MapBoxSearchAreaPainter and MapBoxQuadrilateralPainter.
  • Please, remove huge chunks of commented code.
  • IntersectionTools should not be in ui package.
  • A lot of classes that are unrelated are in ch.epfl.sdp (Auth, DroneMission and a lot of activity classes).
  • MapActivity and OfflineManagerActivity extend MapViewBaseActiviy that is in ui/maps package. Also, both of these are huge. Consider splitting them into fragments (easier to read and maintain the code).

Tests

  • AaaLocationWithoutPermissionTest looks weird. Does this even compile? Is it work in progress?

Layout

  • Avoid hardcoding layout width, height, padding, etc. and use resources instead.
@jovankomatovic
Copy link
Author

jovankomatovic commented May 7, 2020

Hi guys.
We went through your code again and we did not find any new problems.

Overall
The code is fine.
We did not find many problems now.
The architecture of the code is much better than last time.

Code

  • Define constants in app resources.

  • Remove huge chunks of commented code (MapBoxQuadrilateralPainter, Drone).

Tests

  • Again, what is happening with AaaLocationWithoutPermissionTest?

  • Remove commented code (TrajectoryPlanningActivityTest).

Layout

  • Avoid hardcoding layout width, height, padding, etc. and use resources instead.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants