Transform to remove non-revenue stops

[laidig]

Don't need a release cut just for this, but it would be nice to have in master for the next release.

[laidig]

Good catch. This is by design, however, as this particular transform excludes the first and last stops. The other version starts from index 0.

That said, this could be a little cleaner and I'll tweak tomorrow.

On Feb 1, 2017 6:45 PM, "Sean Barbeau" notifications@github.com wrote:

@barbeau requested changes on this pull request.

In onebusaway-gtfs-transformer/src/main/java/org/onebusaway/ gtfs_transformer/updates/RemoveNonRevenueStopsExcluding TerminalsStrategy.java https://github.com/OneBusAway/onebusaway-gtfs-modules/pull/79#pullrequestreview-19686700 :

• *
• */ +public class RemoveNonRevenueStopsExcludingTerminalsStrategy implements GtfsTransformStrategy {
• private static Logger _log = LoggerFactory.getLogger(RemoveNonRevenueStopsExcludingTerminalsStrategy.class);
• @Override
• public void run(TransformContext context, GtfsMutableRelationalDao dao) {
• int removed = 0;
• int total = 0;
• for (Trip trip : dao.getAllTrips()) {
• List stopTimes = dao.getStopTimesForTrip(trip);
• int tripLength = stopTimes.size();
• for (int i =1; i < tripLength; i++) {

I think this should be 0-indexed, otherwise total will be wrong?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/OneBusAway/onebusaway-gtfs-modules/pull/79#pullrequestreview-19686700, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwdPSSnkflD_0MpiJ2d8tnn256_YXd0ks5rYRkhgaJpZM4L0e5W .

[laidig]

@barbeau see changes based on your comments.

[barbeau]

@laidig Comment definitely helps, thanks. I think the total count is still confusing, there, though. Maybe rename it to totalProcessed?

IMHO a less confusing way to structure this is to loop through all stops, and then within the loop have:

if (i == 0 || i == tripLength - 1) {
    // First/last are excluded
    continue;
}

Performance loss is negligible, and while it adds a few lines of code, I think it's much easier to read and reason around. But the total count would change...

[laidig]

Third time's the charm?

On Feb 2, 2017 10:40 AM, "Sean Barbeau" notifications@github.com wrote:

@laidig https://github.com/laidig Comment definitely helps, thanks. I think the total count is still confusing, there, though. Maybe rename it to totalProcessed?

IMHO a less confusing way to structure this is to loop through all stops, and then within the loop have:

if (i == 0 || i == tripLength - 1) { continue; }

Performance loss is negligible, and while it adds a few lines of code, I think it's much easier to read and reason around. But the total count would change...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OneBusAway/onebusaway-gtfs-modules/pull/79#issuecomment-276992155, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwdPcCEwLPIoU5e-h90P5SjqAf5ceeXks5rYfjkgaJpZM4L0e5W .

[barbeau]

:+1:

[laidig]

Sean, could you pull this in? It would be helpful to have this in prod.

[barbeau]

:+1: