Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/nav 64 enrich raptor results inside of raptor to include trips ids etc #51

Conversation

munterfi
Copy link
Member

@munterfi munterfi commented Jun 5, 2024

Add trip information to Raptor Route and store the trip offset in the Raptor legs to be able to reconstruct the connection. Pass trip information on to the Connection of the service and finally map to connection DTO.

Further adds:

  • Some documentation.
  • Application of the visitor in the connection implementation of the service to get the connection arrival and departure time, while removing the redundant information on the public transit leg.
  • Transient to trip on stop info to break the cyclical dependency

munterfi added 11 commits June 5, 2024 11:18
- Only for legs with ArrivalType.ROUTE.
- For other types (INITIAL and TRANSFER) the value is NO_INDEX (=-1).
- RouteBuilder sorts trips according their departure times.
- The order of the trip ids is ensured by the RouteContainer and its LinkedHashMap.

Note: Normal hash maps are not guaranteeing any order.
- Lookup trip id using trip offset in the reconstruction.
… naming in project

- LegType.FOOTPATH is renamed to LegType.WALK_TRANSFER.
- Rename method accessing walk transfer.
…ad of size.

- This seems to be a more useful method on the connection class, to get the number of transfers, we then call size() on the returned list.
- Renamed Num to NumberOf.
- Make trip on stop time transient and exclude from to string.
- Adjust to string of coordinate to match lombok default.
…lic transit leg

Use visitor in connection to get connection departure and arrival time instead as this method seems to be more convenient / user-friendly.
- Set distance as beeline distance between locations.
- Move leg type to first in object, since this important information gets lost otherwise in the full DTO.
@munterfi munterfi requested a review from clukas1 June 5, 2024 12:29
@munterfi munterfi self-assigned this Jun 5, 2024
Copy link
Member

@clukas1 clukas1 left a comment

Choose a reason for hiding this comment

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

Very nice work and very little comments. Three comments that can be ignored (more out of interest questions and opinions), only thing I would change is adding source/targetStop for Walk Legs in the DtoMapper

}

@Override
public Leg visit(Walk walk) {
return new Leg(walk.getSourceLocation(), walk.getTargetLocation(), null, null, LegType.WALK,
return new Leg(LegType.WALK, walk.getSourceLocation(), walk.getTargetLocation(), null, null,
Copy link
Member

Choose a reason for hiding this comment

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

Although not implemented so far, but why not add source or target stop. We have the stop object in our Walk implementation, and can figure out if it it's the source or target by checking the Walk Type First Mile or Last Mile

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I overlooked that. Implemented in: beec21f

@@ -16,4 +16,43 @@ public class ConnectionImpl implements Connection {

private final List<Leg> legs;

@Override
public LocalDateTime getDepartureTime() {
Copy link
Member

Choose a reason for hiding this comment

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

Still not convinced that the visitor is the better solution, than just having the implementation of PublicTransit back referencing the stop time. But I know I'm the minority disagreeing with design patterns, so I can live with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

What bothers you about the visitor? we have no casts and no null values... so it is completely safe ;)

But I know, you really enjoy passing around null values, haha sorry 😄

ch.naviqore.gtfs.schedule.model.Trip gtfsTrip = schedule.getTrips().get(leg.tripId());
Trip trip = map(gtfsTrip, date);

assert gtfsTrip.getStopTimes().size() == trip.getStopTimes()
Copy link
Member

Choose a reason for hiding this comment

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

When or how could this be the case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should never be the case... but since we compare datasets from two different sources (GTFS and Raptor) it could be that we somehow insert inconsistencies. Therefore I added the assert. In production asserts are not executed, so it is really just for us developer to ensure this consistency. Since I guess it could lead to an index out of bounds exception in the best case, in the worst case, the resulting leg is just incorrect without an error.

@munterfi munterfi requested a review from clukas1 June 5, 2024 14:28
Copy link
Member

@clukas1 clukas1 left a comment

Choose a reason for hiding this comment

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

Nice, slowly I'm starting to appreciate the visitor pattern

@clukas1 clukas1 merged commit ae03ed7 into main Jun 5, 2024
2 checks passed
@clukas1 clukas1 deleted the feature/NAV-64-enrich-raptor-results-inside-of-raptor-to-include-trips-ids-etc branch June 6, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants