Prepare for netex import #2584

t2gran · 2018-06-21T12:57:02Z

This PR is based on the PR #2494 - The first commit not in #2494 is:

a86a34b - Prepare Netex import by extracting 'trip pattern generation' and 'stop times repair' out of GTFSPatternHopFactory.java (Currently the only one in this PR - after 2494 is merged)

This commit should be merged into OTP some time before we start cleaning up the code as preparation for OTP 2.0.

If the community approve we can make it part of 1.4, but I think the best "place" for it is as one of the first commits after release 1.4 and in beta 2.0.

The Goal for this refactoring is to isolate:

trip pattern generation
stop times repair

Both of this is currently done in the PatternHopFactory(PHF). We want to reuse the PHF when importing NeTEx data, and these 2 features are not requierd for our NeTEx import.

drewda · 2018-08-10T20:19:20Z

@t2gran now that 1.3 is released, it would be good to return to this PR and prepare to merge it for 1.4. Can you update this branch so that it can be clearly merged?

@sdjacobs are you available to review this PR? I believe in Boston we all discussed how it may overlap with changes you make to onebusaway-gtfs in #2603.

t2gran · 2018-08-13T17:31:41Z

Please review PR #2494 first.

drewda · 2018-08-13T19:12:05Z

@t2gran O.K., I've put #2494 as next up on the roadmap for v1.4, and I've moved this to the v2.0-rc column.

t2gran · 2018-08-14T11:43:00Z

Note! I have rebased this on top of the latest changes to PR 2494.

…p times repair' out of GTFSPatternHopFactory.java

t2gran · 2019-04-08T17:35:11Z

@abyrd I have rebased this on top of dev-2.x, it is ready for review.

abyrd

I'm submitting this review now for discussion during our upcoming call. Overall I'd say it is in a decent state to be merged, to pave the way for completing Netex import. However I would like to add a few more Javadoc comments and do a bit of refactoring together before that merge, and plan in the longer term to revisit and refactor this for maintainability once we've got both GTFS and Netex import working in the main dev-2.x branch.

src/main/java/org/opentripplanner/routing/edgetype/factory/PatternHopFactory.java

abyrd · 2019-04-25T04:42:14Z

src/main/java/org/opentripplanner/model/impl/OtpTransitServiceImpl.java

-            for (List<ShapePoint> list : shapePointsByShapeId.values()) {
-                Collections.sort(list);
-            }
+    private static <T> List<T> nullSafeUnmodifiableList(Collection<T> c) {


This could use some Javadoc stating the purpose of the method. The implementation seems to do something much more general than being null-safe: it ignores any value that is not a List, converting it to an empty list. So if someone passed in a Set it would become an empty List. If the only expected cases are c instanceof List and c == null then all other situations should be caught be a check that throws an exception.

list = new ArrayList<>(c) creates a new List with all elements passed in.

It actually convert a Set to a List.

Oh right, I somehow missed that c parameter. So it will handle any kind of collection. But then it is not null-safe in the sense that it will tolerate c == null right? This ArrayList constructor's Javadoc says: c – the collection whose elements are to be placed into this list. Throws: NullPointerException – if the specified collection is null

You are right, the null-safety is probably survived trough some refactorings - I will rename the method: immutableList and add some JavaDoc.

src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java

abyrd · 2019-04-25T04:55:31Z

src/main/java/org/opentripplanner/model/impl/OtpTransitServiceImpl.java


    /**
     * Create a read only version of the {@link OtpTransitService}.
-     * @see OtpTransitServiceBuilder Use builder to mutate the instance.
+     *
+     * @see OtpTransitServiceBuilder Use builder to create an new OtpTransitDao.


There seems to be some incomplete renaming here: a reference to OtpTransitDao. I'm not sure "service" is better than "DAO" here but it looks like "service" is being used consistently. We can do a big renaming session later once things are merged.

How do you want to keep track of stuff we need to do - that is not big enough to deserve its own issue?
Possibilities:

Create an issue with a bullet point for each minor thing. We can add things to it by editing the description.

Add TODOs in the code. I usually add TODO {inisials} - {description} or TODO {tag} - {description}. We can use OTP2 as a tag.

A combination: Add an issue to fix all TODOs with our names/tag(OTP2) on it.

Any time we defer some batch of changes, we should record them somewhere. I think an issue with a checklist in its description would work.

Like

This

I created a new issue for this #2757

abyrd · 2019-04-25T05:01:01Z

src/main/java/org/opentripplanner/model/impl/EntityMap.java

+import java.util.HashMap;
+import java.util.Map;
+
+public class EntityMap<I extends Serializable, E extends IdentityBean<I>> {


All new classes should have Javadoc explaining purpose, usage, invariants and constraints etc. Perhaps this has been factored out of some pre-existing class, but I did a quick search and did not find it.

As always I am happy to assist in writing, revising or expanding Javadoc. I can work with you on adding such descriptions during our work sessions.

I have added JavaDoc to this one.

src/main/java/org/opentripplanner/routing/edgetype/factory/BlockIdAndServiceId.java

abyrd · 2019-04-25T08:01:45Z

src/main/java/org/opentripplanner/routing/edgetype/factory/PatternHopFactory.java


    public PatternHopFactory(GtfsContext context) {
        this.feedId = context.getFeedId();
-        this.transitService = context.getOtpTransitService();
-        this.calendarService = context.getCalendarService();
+        this.transitService = context.getTransitBuilder().build();
    }

    public PatternHopFactory() {


This constructor appears to be unused, and I don't see any good reason for it. We should remove it.

abyrd · 2019-04-25T08:19:26Z

src/main/java/org/opentripplanner/model/impl/OtpTransitServiceBuilder.java

    }

    public List<ShapePoint> getShapePoints() {
        return shapePoints;
    }

-    public List<Stop> getStops() {
-        return stops;
+    public EntityMap<FeedScopedId, Stop> getStops() {


It feels wrong to me to have getters on a Builder class. I realize this has been adapted from a pre-existing class that had getters. But I see several of these getters being used to fetch a collection, on which the caller then calls add(). It would make more sense to replace these getters with methods like addStops() or addRoutes().

However, I see that in some places like GtfsContextBuilder.build() we are actually fetching collections from inside this Builder and modifying them. I see that this is just a way of progressively refactoring existing code, but it seems like it could be further refactored to have more conventional behavior.

I just tried to be consistent, but I see your point. I think is is vice to leave it for now and return to it after we have applied the NeTEx import - which (I am afraid) takes this problem to a whole new level (-;

For the record, there are several solutions:

making immutable access til list and add explicit add methods

making a custom "List" list like the EntityMap that have methods for adding and immutable iteration.

Also, maybe the "builder/service" names is wrong - and we should spend some time modeling this properly.

Consistency is of course an important goal as well. But in this case it seems like getters on a builder are exceptions - necessary deviations from an ideal model. So maybe the getter should have mostly additive / constructive functions, and only a few exceptional getters where the current design happens to require it.

I agree with your three points here. Both of the first two could make the code easier to reason about, and it's interesting to consider that Builder is simply not the right name/concept here. Actually we've got more of a mutable and immutable version of the same object. When I look at it that way, the boilerplate repetition seems kind of excessive. The existence of mutable builders and immutable output classes from builders seems better justified to me, in that the Builder also provides a convenient fluent API for object preparation.

Let's rethink only after we've got the existing Netex handlers merged in.

abyrd · 2019-04-25T08:23:28Z

src/test/java/org/opentripplanner/gtfs/GtfsContextBuilder.java

+     * contextBuilder.repairStopTimesAndGenerateTripPatterns();
+     * </pre>
+     */
+    public void repairStopTimesAndGenerateTripPatterns() {


I like that the stop time repair is factored out and made optional. However, it feels strange that this is done in GtfsContextBuilder, and that this class is reaching into an unfinished builder to carry out all these modifications at the moment of its build() operation. I see how it has evolved from the existing code. But again I hope we can eventually further refactor this to have more conventional behavior.

src/test/java/org/opentripplanner/calendar/impl/CalendarServiceDataFactoryImplTest.java

t2gran · 2019-04-25T11:41:52Z

src/main/java/org/opentripplanner/model/impl/SortedMultimap.java

+    public void reindex() {
+        HashMap<K, List<V>> temp = new HashMap<>(map);
+        map.clear();
+        map.putAll(temp);


The reindex need a comment - I had to think hard for a long time why this is needed and that it works.

on reindex() to document the intended behavior.

- Add unit test on reindex() to document the intended behavior. - Rename methods ´asMap()´ to more specific `asImmutableMap`, and add JavaDoc. - Rename `EntityMap` to `EntityById`.

- Add comments on side-effects in GtfsModule.

- Update JavaDoc.

t2gran · 2019-04-25T18:48:04Z

@abyrd I am done fixing the all smaller issues you have commented on. The rest I would like to get back to after the NeTEx import it self.

abyrd

As discussed in yesterday's conference call, the code looks good from a functional point of view and can be merged. There are some style issues that we plan to debate and fix later, for which we will create additional tickets.

One thing I noticed but didn't mention yesterday: GTFSToOtpTransitServiceMapper has no persistent state, so it does not need to be instantiated. If you move all the fields into the main mapping method as local variables, then merge its two methods into a single public static OtpTransitServiceBuilder mapGtfsDaoToInternalTransitServiceBuilder (GtfsRelationalDao data) { ... } it works as a static factory method.

src/main/java/org/opentripplanner/graph_builder/module/GtfsModule.java

src/main/java/org/opentripplanner/model/impl/EntityById.java

abyrd · 2019-04-26T10:10:35Z

src/main/java/org/opentripplanner/model/TripStopTimes.java

+     * Return a copy of the internal map. Changes in the source are not reflected
+     * in the destination (returned Map), and visa versa.
+     * <p/>
+     * The returned map is immutable.


Although the map is immutable, the values of the map will be mutable lists unless additional precautions are taken. The addAll and sort() methods could be modified to make all the value Lists immutable as well.

src/main/java/org/opentripplanner/model/impl/EntityById.java

t2gran · 2019-04-26T13:08:31Z

The GTFSToOtpTransitServiceMapper was intensionally written this way to make a separation of creating mappers (needed to do the mapping) and the mapping it self. This enables the mapping method to be refactored later into several methods without passing local variables. Also this way it is clear that no mapper depend on the input data to be mapped - they are all created BEFORE the data is available. But I guess it is a matter of style. Let me know if you want me to change it. Another point worth mentioned is that the constructor is private - witch guarantees that the local mappers are created for each mapping request (same as local variables).

- Rename 'nullSafeUnmodifiableList' to 'immutableList' and add a java doc.

- Remove unused constructor in PatternHopFactory.

abyrd

Thanks for all the fixes and responses.

drewda added this to v1.4 in OpenTripPlanner 1.x and 2.x roadmap Jul 18, 2018

drewda requested review from sdjacobs and a team August 10, 2018 20:17

drewda moved this from v1.4 to v2.0-rc in OpenTripPlanner 1.x and 2.x roadmap Aug 13, 2018

drewda removed request for a team and sdjacobs August 13, 2018 19:10

t2gran force-pushed the prepare_for_netex_import branch from a86a34b to c4c680e Compare August 14, 2018 11:34

t2gran requested a review from a team as a code owner August 14, 2018 11:34

t2gran mentioned this pull request Aug 16, 2018

Refactor OTP to have new OTP classes to replace the OBA GTFS classes #2495

Merged

t2gran changed the base branch from master to dev-2.x April 8, 2019 15:02

Prepare Netex import by extracting 'trip pattern generation' and 'sto…

ef9d444

…p times repair' out of GTFSPatternHopFactory.java

t2gran force-pushed the prepare_for_netex_import branch from c4c680e to ef9d444 Compare April 8, 2019 17:33

t2gran added the 2.x label Apr 9, 2019

abyrd requested changes Apr 25, 2019

View reviewed changes

t2gran commented Apr 25, 2019

View reviewed changes

t2gran added 2 commits April 25, 2019 15:55

Convert SortedMultimap to its only usage -> TripStopTimes. Add unit test

26c40ea

on reindex() to document the intended behavior.

Fix some of the review comments in PR # 2584.

5671d2e

- Add unit test on reindex() to document the intended behavior. - Rename methods ´asMap()´ to more specific `asImmutableMap`, and add JavaDoc. - Rename `EntityMap` to `EntityById`.

t2gran force-pushed the prepare_for_netex_import branch from 719f1b2 to 5671d2e Compare April 25, 2019 18:02

t2gran added 2 commits April 25, 2019 20:18

Fix some of the review comments in PR opentripplanner#2584.

15f7b9a

- Add comments on side-effects in GtfsModule.

Fix some of the review comments in PR opentripplanner#2584.

8c3fcba

- Update JavaDoc.

abyrd previously approved these changes Apr 26, 2019

View reviewed changes

Fix some of the review comments in PR opentripplanner#2584.

723cf72

- Rename 'nullSafeUnmodifiableList' to 'immutableList' and add a java doc.

t2gran dismissed abyrd’s stale review via 723cf72 April 26, 2019 13:32

t2gran mentioned this pull request Apr 26, 2019

Final cleanup OTP2 - before release #2757

Closed

14 tasks

Fix some of the review comments in PR opentripplanner#2584.

1a4819f

- Remove unused constructor in PatternHopFactory.

abyrd approved these changes May 2, 2019

View reviewed changes

abyrd merged commit 136091e into opentripplanner:dev-2.x May 2, 2019

t2gran deleted the prepare_for_netex_import branch September 24, 2019 14:19

abyrd added this to the 2.0 milestone Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare for netex import #2584

Prepare for netex import #2584

t2gran commented Jun 21, 2018 •

edited

drewda commented Aug 10, 2018

t2gran commented Aug 13, 2018 •

edited

drewda commented Aug 13, 2018

t2gran commented Aug 14, 2018

t2gran commented Apr 8, 2019

abyrd left a comment

abyrd Apr 25, 2019

t2gran Apr 25, 2019

t2gran Apr 25, 2019

abyrd Apr 26, 2019

t2gran Apr 26, 2019

abyrd Apr 25, 2019

t2gran Apr 25, 2019

t2gran Apr 25, 2019 •

edited

abyrd Apr 26, 2019

t2gran Apr 26, 2019

abyrd Apr 25, 2019

t2gran Apr 25, 2019

abyrd Apr 25, 2019

abyrd Apr 25, 2019

t2gran Apr 25, 2019

abyrd Apr 26, 2019

abyrd Apr 25, 2019

t2gran Apr 25, 2019

t2gran commented Apr 25, 2019

abyrd left a comment

abyrd Apr 26, 2019

t2gran commented Apr 26, 2019

abyrd left a comment

Prepare for netex import #2584

Prepare for netex import #2584

Conversation

t2gran commented Jun 21, 2018 • edited

drewda commented Aug 10, 2018

t2gran commented Aug 13, 2018 • edited

drewda commented Aug 13, 2018

t2gran commented Aug 14, 2018

t2gran commented Apr 8, 2019

abyrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2gran Apr 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2gran commented Apr 25, 2019

abyrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2gran commented Apr 26, 2019

abyrd left a comment

Choose a reason for hiding this comment

t2gran commented Jun 21, 2018 •

edited

t2gran commented Aug 13, 2018 •

edited

t2gran Apr 25, 2019 •

edited