Refactor nearby classes mvp #2969
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,8 +28,10 @@ | |
| public class NearbyController { | ||
|
There was a problem hiding this comment. Now that we have a presenter for Nearby, we should remove this class and shift the functionalities to the presenter There was a problem hiding this comment. Nearby controller controls our map logic (ie. how wide area is queried) while NearbyPresenter controls fragments and views. So I think we should keep them separated, but renaming NearbyController can be a better approach to eliminate confusion. What do you say @ashishkumar468 ? There was a problem hiding this comment. @neslihanturan any logic, be it controlling the map logic should go in the corresponding view presenter, we should not ideally have classes other than the presenter to have logics. If we talk in terms of unit testing, it would just be the presenter which would need to be tested. There was a problem hiding this comment. Hmm, but I can say that these two classes has really different responsibilities. Merging them not only makes the presenter class too long but also causes a semantic confusion. NearbyController decides which places will be displayed and what their included information is while presenter decides how to display them. There was a problem hiding this comment. The presenter is solely responsible for deciding how to display a view. If the view is complex then the presenter will become longer but the controller doesn't fit into the MVP semantics. If you are inclined towards having smaller presenters, you can try to break down the views into smaller logical views and have presenters for each of them. There was a problem hiding this comment. Can you please read what does NearbyController method do, it does not intended to control views, it does not decide how to display views either. It controls our search operation. It controls how we fetch data from Wikidata. Ie. storing our previously searched radius, our max number of results and accordingly giving us list of data in form of lets say Place type. It is not the same thing which is described in MVP documentation IMO. What do you say? |
||
| private static final int MAX_RESULTS = 1000; | ||
| private final NearbyPlaces nearbyPlaces; | ||
| public static double currentLocationSearchRadius = 10.0; //in kilometers | ||
| public static LatLng currentLocation; // Users latest fetched location | ||
| public static LatLng latestSearchLocation; // Can be current and camera target on search this area button is used | ||
| public static double latestSearchRadius = 10.0; // Any last search radius except closest result search | ||
|
|
||
| @Inject | ||
| public NearbyController(NearbyPlaces nearbyPlaces) { | ||
|
|
@@ -41,21 +43,21 @@ public NearbyController(NearbyPlaces nearbyPlaces) { | |
| * Prepares Place list to make their distance information update later. | ||
| * | ||
| * @param curLatLng current location for user | ||
| * @param searchLatLng the location user wants to search around | ||
| * @param returnClosestResult if this search is done to find closest result or all results | ||
| * @return NearbyPlacesInfo a variable holds Place list without distance information | ||
| * and boundary coordinates of current Place List | ||
| */ | ||
| public NearbyPlacesInfo loadAttractionsFromLocation(LatLng curLatLng, LatLng searchLatLng, boolean returnClosestResult, boolean checkingAroundCurrentLocation) throws IOException { | ||
|
|
||
| Timber.d("Loading attractions near %s", searchLatLng); | ||
| NearbyPlacesInfo nearbyPlacesInfo = new NearbyPlacesInfo(); | ||
|
|
||
| if (searchLatLng == null) { | ||
| Timber.d("Loading attractions nearby, but curLatLng is null"); | ||
| return null; | ||
| } | ||
| List<Place> places = nearbyPlaces.radiusExpander(searchLatLng, Locale.getDefault().getLanguage(), returnClosestResult); | ||
|
|
||
| if (null != places && places.size() > 0) { | ||
| LatLng[] boundaryCoordinates = {places.get(0).location, // south | ||
|
|
@@ -91,13 +93,25 @@ public NearbyPlacesInfo loadAttractionsFromLocation(LatLng curLatLng, LatLng lat | |
| } | ||
| ); | ||
| } | ||
| nearbyPlacesInfo.curLatLng = curLatLng; | ||
| nearbyPlacesInfo.searchLatLng = searchLatLng; | ||
| nearbyPlacesInfo.placeList = places; | ||
| nearbyPlacesInfo.boundaryCoordinates = boundaryCoordinates; | ||
|
|
||
| // Returning closes result means we use the controller for nearby card. So no need to set search this area flags | ||
| if (!returnClosestResult) { | ||
| // To remember latest search either around user or any point on map | ||
| latestSearchLocation = searchLatLng; | ||
| latestSearchRadius = nearbyPlaces.radius*1000; // to meter | ||
|
|
||
| // Our radius searched around us, will be used to understand when user search their own location, we will follow them | ||
| if (checkingAroundCurrentLocation) { | ||
| currentLocationSearchRadius = nearbyPlaces.radius*1000; // to meter | ||
| currentLocation = curLatLng; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| return nearbyPlacesInfo; | ||
| } | ||
| else { | ||
|
|
@@ -112,7 +126,7 @@ public NearbyPlacesInfo loadAttractionsFromLocation(LatLng curLatLng, LatLng lat | |
| * @param placeList list of nearby places in Place data type | ||
| * @return Place list that holds nearby places | ||
| */ | ||
| public static List<Place> loadAttractionsFromLocationToPlaces( | ||
| LatLng curLatLng, | ||
| List<Place> placeList) { | ||
| placeList = placeList.subList(0, Math.min(placeList.size(), MAX_RESULTS)); | ||
|
|
@@ -212,5 +226,7 @@ public static List<NearbyBaseMarker> loadAttractionsFromLocationToBaseMarkerOpti | |
| public class NearbyPlacesInfo { | ||
| public List<Place> placeList; // List of nearby places | ||
| public LatLng[] boundaryCoordinates; // Corners of nearby area | ||
| public LatLng curLatLng; // Current location when this places are populated | ||
| public LatLng searchLatLng; // Search location for finding this places | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we use callbacks for this thing, that way code would be cleaner and by defining contracts even novice contributors would know the intended use case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree with @ashishkumar468. Let us use callbacks.