Skip to content

Conversation

@danesfeder
Copy link
Contributor

Closes #932

@danesfeder danesfeder added bug Defect to be fixed. navigation-core labels May 15, 2018
@danesfeder danesfeder added this to the 0.14.0 milestone May 15, 2018
@danesfeder danesfeder self-assigned this May 15, 2018
List<Pair<StepIntersection, Double>> distancesToIntersections = new ArrayList<>();
List<StepIntersection> stepIntersections = new ArrayList<>(intersections);
if (stepPoints.isEmpty()) {
if (stepPoints.size() < TWO_POINTS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about consolidating these two guard clauses 👇 (that return the same) extracting the conditional into a explaining variable?
Also what about creating stepIntersections after the guard clauses?

Source code structure
Variables and methods should be defined close to where they are used.
Local variables should be declared just above their first usage and should
have a small vertical scope.

Clean Code (Robert C. Martin)

That way we avoid allocating it if any of the guard clauses is executed.

boolean invalidPointList = stepPoints.size() < TWO_POINTS;
boolean emptyIntersectionList = stepIntersections.isEmpty();
if (invalidPointList || emptyIntersectionList) {
return distancesToIntersections;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we return Collections.emptyList() to make it more explicit? If so we can move define distancesToIntersections down, close to where it's used. BTW we should check if there's any implication on returning an immutable list in this case 👀 https://stackoverflow.com/questions/5552258/collections-emptylist-vs-new-instance#answer-5552287

@@ -299,10 +300,9 @@ public static List<Pair<StepIntersection, Double>> createDistancesToIntersection
List<StepIntersection> intersections) {
List<Pair<StepIntersection, Double>> distancesToIntersections = new ArrayList<>();
List<StepIntersection> stepIntersections = new ArrayList<>(intersections);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we creating stepIntersections here? We could check if intersections is empty (instead of stepIntersections.isEmpty()) and define it down (after the guard clause), close to where it's used. Am I missing something?

@danesfeder danesfeder force-pushed the dan-line-slice branch 2 times, most recently from e7b121e to 14e083e Compare May 16, 2018 13:19
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@danesfeder danesfeder merged commit fa526ad into master May 16, 2018
@danesfeder danesfeder deleted the dan-line-slice branch May 16, 2018 14:01
@danesfeder danesfeder mentioned this pull request May 31, 2018
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Defect to be fixed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants