Check Turn / Then Banner on each update #696
Conversation
danesfeder
commented
Feb 12, 2018
danesfeder
commented
- Previously would only check when new step was found
- Fixes bug with then banner not showing after medium alert level
| @@ -98,7 +98,7 @@ private void thenStep(LegStep upcomingStep, LegStep followOnStep, double current | |||
| thenStepManeuverType = followOnStep.maneuver().type(); | |||
| thenStepManeuverModifier = followOnStep.maneuver().modifier(); | |||
| // Should show then step if the upcoming step is less than 25 seconds | |||
There was a problem hiding this comment.
This comment is now lying. This is an example of why I'm not a big fan of comments 
There was a problem hiding this comment.
The best comment is a good name for a method or class.
or variable in this case 😛
👀 Code Smells 👉 Comments
| @@ -98,7 +98,7 @@ private void thenStep(LegStep upcomingStep, LegStep followOnStep, double current | |||
| thenStepManeuverType = followOnStep.maneuver().type(); | |||
| thenStepManeuverModifier = followOnStep.maneuver().modifier(); | |||
| // Should show then step if the upcoming step is less than 25 seconds | |||
| shouldShowThenStep = upcomingStep.duration() <= (25d * 1.2d) && currentDurationRemaining <= (70d * 1.2d); | |||
| shouldShowThenStep = upcomingStep.duration() <= (25d * 1.2d) && currentDurationRemaining <= 70d; | |||
There was a problem hiding this comment.
Could we fix the variable name to reflect what actually is doing? This way comment will become superfluous so it won't be necessary.
There was a problem hiding this comment.
What about decomposing conditional extracting that complicated expression into a well-named private method? Conditionals tend to get more and more complicated in their logic over time.
| String maneuverViewModifier = model.getStepResources().getManeuverViewModifier(); | ||
| double durationRemaining = model.getProgress().currentLegProgress().currentStepProgress().durationRemaining(); | ||
|
|
||
| if (turnLanes != null && !TextUtils.isEmpty(maneuverViewModifier) && durationRemaining <= 70d) { |
There was a problem hiding this comment.
What about simplifying this conditional? 👀 Decompose Conditional
- Allows us to show after medium alert level duration remaining
danesfeder
force-pushed
the
dan-delay-then-banner
branch
from
14c9d64
to
dd4ebd6
Compare
|
@Guardiola31337 Comments addressed 🍍 |
| shouldShowThenStep = validStepDuration(upcomingStep, currentDurationRemaining); | ||
| } | ||
|
|
||
| private boolean validStepDuration(LegStep upcomingStep, double currentDurationRemaining) { |
There was a problem hiding this comment.
Minor detail, what about isValidStepDuration instead?
