diff --git a/__tests__/util/__mocks__/itinerary.json b/__tests__/util/__mocks__/itinerary.json new file mode 100644 index 000000000..545c9dd28 --- /dev/null +++ b/__tests__/util/__mocks__/itinerary.json @@ -0,0 +1,91 @@ +{ + "route1": { + "longName": "Across town", + "mode": "BUS", + "shortName": "10", + "sortOrder": 10 + }, + "route2": { + "longName": "Around town", + "mode": "BUS", + "shortName": "20", + "sortOrder": 2 + }, + "route3": { + "longName": "Around another town", + "mode": "BUS", + "shortName": "3", + "sortOrder": -999 + }, + "route4": { + "longName": "Loop route", + "mode": "BUS", + "shortName": "2", + "sortOrder": -999 + }, + "route5": { + "longName": "A-line", + "mode": "BUS", + "shortName": "A", + "sortOrder": -999 + }, + "route6": { + "longName": "B-line", + "mode": "BUS", + "shortName": "B", + "sortOrder": -999 + }, + "route7": { + "longName": "A meandering route", + "mode": "BUS", + "sortOrder": -999 + }, + "route8": { + "longName": "Zig-zagging route", + "mode": "BUS", + "shortName": "30", + "sortOrder": 2 + }, + "route9": { + "longName": "Express route", + "mode": "BUS", + "shortName": "30", + "sortOrder": 2 + }, + "route10": { + "longName": "Variation of express route", + "mode": "BUS", + "shortName": "30", + "sortOrder": 2 + }, + "route11": { + "longName": "Local route", + "mode": "BUS", + "shortName": "6", + "sortOrder": 2 + }, + "route12": { + "longName": "Intercity Train", + "mode": "RAIL", + "shortName": "IC", + "sortOrder": 2 + }, + "route13": { + "longName": "Yellow line Subway", + "mode": "SUBWAY", + "shortName": "Yellow", + "sortOrder": 2 + }, + "route14": { + "longName": "Xpress route C", + "mode": "BUS", + "shortName": "30C", + "sortOrder": 2 + }, + "route15": { + "longName": "Express route X", + "mode": "BUS", + "shortName": "30X", + "sortOrder": 2 + } +} diff --git a/__tests__/util/__snapshots__/itinerary.js.snap b/__tests__/util/__snapshots__/itinerary.js.snap new file mode 100644 index 000000000..13af42b1d --- /dev/null +++ b/__tests__/util/__snapshots__/itinerary.js.snap @@ -0,0 +1,247 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`util > itinerary routeComparator should prioritize routes with integer shortNames over alphabetic shortNames 1`] = ` +Array [ + Object { + "longName": "A-line", + "mode": "BUS", + "shortName": "A", + "sortOrder": -999, + }, + Object { + "longName": "Loop route", + "mode": "BUS", + "shortName": "2", + "sortOrder": -999, + }, +] +`; + +exports[`util > itinerary routeComparator should prioritize routes with shortNames over those with just longNames 1`] = ` +Array [ + Object { + "longName": "B-line", + "mode": "BUS", + "shortName": "B", + "sortOrder": -999, + }, + Object { + "longName": "A meandering route", + "mode": "BUS", + "sortOrder": -999, + }, +] +`; + +exports[`util > itinerary routeComparator should prioritize routes with valid sortOrder 1`] = ` +Array [ + Object { + "longName": "Around town", + "mode": "BUS", + "shortName": "20", + "sortOrder": 2, + }, + Object { + "longName": "Around another town", + "mode": "BUS", + "shortName": "3", + "sortOrder": -999, + }, +] +`; + +exports[`util > itinerary routeComparator should sort based off of route type 1`] = ` +Array [ + Object { + "longName": "Yellow line Subway", + "mode": "SUBWAY", + "shortName": "Yellow", + "sortOrder": 2, + }, + Object { + "longName": "Intercity Train", + "mode": "RAIL", + "shortName": "IC", + "sortOrder": 2, + }, +] +`; + +exports[`util > itinerary routeComparator should sort routes based off of integer shortName 1`] = ` +Array [ + Object { + "longName": "Loop route", + "mode": "BUS", + "shortName": "2", + "sortOrder": -999, + }, + Object { + "longName": "Around another town", + "mode": "BUS", + "shortName": "3", + "sortOrder": -999, + }, +] +`; + +exports[`util > itinerary routeComparator should sort routes based off of longNames 1`] = ` +Array [ + Object { + "longName": "Express route", + "mode": "BUS", + "shortName": "30", + "sortOrder": 2, + }, + Object { + "longName": "Variation of express route", + "mode": "BUS", + "shortName": "30", + "sortOrder": 2, + }, +] +`; + +exports[`util > itinerary routeComparator should sort routes based off of shortNames 1`] = ` +Array [ + Object { + "longName": "A-line", + "mode": "BUS", + "shortName": "A", + "sortOrder": -999, + }, + Object { + "longName": "B-line", + "mode": "BUS", + "shortName": "B", + "sortOrder": -999, + }, +] +`; + +exports[`util > itinerary routeComparator should sort routes based off of sortOrder 1`] = ` +Array [ + Object { + "longName": "Around town", + "mode": "BUS", + "shortName": "20", + "sortOrder": 2, + }, + Object { + "longName": "Across town", + "mode": "BUS", + "shortName": "10", + "sortOrder": 10, + }, +] +`; + +exports[`util > itinerary routeComparator should sort routes on all of the criteria at once 1`] = ` +Array [ + Object { + "longName": "Yellow line Subway", + "mode": "SUBWAY", + "shortName": "Yellow", + "sortOrder": 2, + }, + Object { + "longName": "Intercity Train", + "mode": "RAIL", + "shortName": "IC", + "sortOrder": 2, + }, + Object { + "longName": "Local route", + "mode": "BUS", + "shortName": "6", + "sortOrder": 2, + }, + Object { + "longName": "Around town", + "mode": "BUS", + "shortName": "20", + "sortOrder": 2, + }, + Object { + "longName": "Express route", + "mode": "BUS", + "shortName": "30", + "sortOrder": 2, + }, + Object { + "longName": "Variation of express route", + "mode": "BUS", + "shortName": "30", + "sortOrder": 2, + }, + Object { + "longName": "Zig-zagging route", + "mode": "BUS", + "shortName": "30", + "sortOrder": 2, + }, + Object { + "longName": "Xpress route C", + "mode": "BUS", + "shortName": "30C", + "sortOrder": 2, + }, + Object { + "longName": "Express route X", + "mode": "BUS", + "shortName": "30X", + "sortOrder": 2, + }, + Object { + "longName": "Across town", + "mode": "BUS", + "shortName": "10", + "sortOrder": 10, + }, + Object { + "longName": "A-line", + "mode": "BUS", + "shortName": "A", + "sortOrder": -999, + }, + Object { + "longName": "B-line", + "mode": "BUS", + "shortName": "B", + "sortOrder": -999, + }, + Object { + "longName": "Loop route", + "mode": "BUS", + "shortName": "2", + "sortOrder": -999, + }, + Object { + "longName": "Around another town", + "mode": "BUS", + "shortName": "3", + "sortOrder": -999, + }, + Object { + "longName": "A meandering route", + "mode": "BUS", + "sortOrder": -999, + }, +] +`; + +exports[`util > itinerary routeComparator should sort routes with alphanumeric shortNames 1`] = ` +Array [ + Object { + "longName": "Xpress route C", + "mode": "BUS", + "shortName": "30C", + "sortOrder": 2, + }, + Object { + "longName": "Express route X", + "mode": "BUS", + "shortName": "30X", + "sortOrder": 2, + }, +] +`; diff --git a/__tests__/util/itinerary.js b/__tests__/util/itinerary.js index 6c1f4852e..74e5d5a56 100644 --- a/__tests__/util/itinerary.js +++ b/__tests__/util/itinerary.js @@ -1,7 +1,88 @@ -import {isTransit} from '../../lib/util/itinerary' +import {isTransit, routeComparator} from '../../lib/util/itinerary' + +const { + route1, + route2, + route3, + route4, + route5, + route6, + route7, + route8, + route9, + route10, + route11, + route12, + route13, + route14, + route15 +} = require('./__mocks__/itinerary.json') + +function sortRoutes (...routes) { + routes.sort(routeComparator) + return routes +} describe('util > itinerary', () => { it('isTransit should work', () => { expect(isTransit('CAR')).toBeFalsy() }) + + describe('routeComparator', () => { + it('should sort routes based off of sortOrder', () => { + expect(sortRoutes(route1, route2)).toMatchSnapshot() + }) + + it('should prioritize routes with valid sortOrder', () => { + expect(sortRoutes(route2, route3)).toMatchSnapshot() + }) + + it('should sort routes based off of integer shortName', () => { + expect(sortRoutes(route3, route4)).toMatchSnapshot() + }) + + it('should prioritize routes with integer shortNames over alphabetic shortNames', () => { + expect(sortRoutes(route4, route5)).toMatchSnapshot() + }) + + it('should sort routes based off of shortNames', () => { + expect(sortRoutes(route5, route6)).toMatchSnapshot() + }) + + it('should sort routes with alphanumeric shortNames', () => { + expect(sortRoutes(route14, route15)).toMatchSnapshot() + }) + + it('should prioritize routes with shortNames over those with just longNames', () => { + expect(sortRoutes(route6, route7)).toMatchSnapshot() + }) + + it('should sort routes based off of longNames', () => { + expect(sortRoutes(route9, route10)).toMatchSnapshot() + }) + + it('should sort routes on all of the criteria at once', () => { + expect(sortRoutes( + route1, + route2, + route3, + route4, + route5, + route6, + route7, + route8, + route9, + route10, + route11, + route12, + route13, + route14, + route15 + )).toMatchSnapshot() + }) + + it('should sort based off of route type', () => { + expect(sortRoutes(route12, route13)).toMatchSnapshot() + }) + }) }) diff --git a/lib/components/viewers/route-viewer.js b/lib/components/viewers/route-viewer.js index 2c41f3fac..8fe775cda 100644 --- a/lib/components/viewers/route-viewer.js +++ b/lib/components/viewers/route-viewer.js @@ -57,7 +57,9 @@ class RouteViewer extends Component { setViewedRoute, viewedRoute } = this.props - const sortedRoutes = routes ? Object.values(routes).sort(routeComparator) : [] + const sortedRoutes = routes + ? Object.values(routes).sort(routeComparator) + : [] const agencySortedRoutes = operators.length > 0 ? sortedRoutes.sort((a, b) => { return operatorIndexForRoute(operators, a) - operatorIndexForRoute(operators, b) diff --git a/lib/util/itinerary.js b/lib/util/itinerary.js index 024800df6..ed14e0489 100644 --- a/lib/util/itinerary.js +++ b/lib/util/itinerary.js @@ -245,31 +245,219 @@ export function getLegBounds (leg) { return latLngBounds(coords) } -export function routeComparator (a, b) { - let aComp, bComp - // If route sort order field is available, default sort to these predefined - // values. NOTE: OTP/OBA defaults sort order values to -999, so instead of - // checking that the sort order is not null, we just check that the values are - // not equal. Technically, there could be cases where route_sort_order values - // are equivalent, but ideally they should be unique. - if (a.sortOrder !== b.sortOrder) { - aComp = a.sortOrder - bComp = b.sortOrder - } else if (!isNaN(parseInt(a.shortName)) && !isNaN(parseInt(b.shortName))) { - // Otherwise, if both short names can be parsed as integers, use these - // numbers for sorting. - aComp = parseInt(a.shortName) - bComp = parseInt(b.shortName) +/** + * Gets the desired sort values according to an optional getter function. If the + * getter function is not defined, the original sort values are returned. + */ +function getSortValues (getterFn, a, b) { + let aVal + let bVal + if (typeof getterFn === 'function') { + aVal = getterFn(a) + bVal = getterFn(b) } else { - // Otherwise, default to short or long name strings. - aComp = a.shortName || a.longName - bComp = b.shortName || b.longName + aVal = a + bVal = b + } + return { aVal, bVal } +} + +/** + * Gets a comparator value for a given route's type (OTP mode). + */ +function getRouteTypeComparatorValue (route) { + switch (route.mode) { + case 'SUBWAY': + return 1 + case 'TRAM': + return 2 + case 'RAIL': + return 3 + case 'GONDOLA': + return 4 + case 'FERRY': + return 5 + case 'CABLE_CAR': + return 6 + case 'FUNICULAR': + return 7 + case 'BUS': + return 8 + default: + return 9999 } - if (aComp < bComp) return -1 - if (aComp > bComp) return 1 +} + +/** + * Calculates the sort comparator value given two routes based off of route type + * (OTP mode). + */ +function routeTypeComparator (a, b) { + return getRouteTypeComparatorValue(a) - getRouteTypeComparatorValue(b) +} + +/** + * Determines whether a value is a string that starts with an alphabetic + * ascii character. + */ +function startsWithAlphabeticCharacter (val) { + if (typeof val === 'string' && val.length > 0) { + const firstCharCode = val.charCodeAt(0) + return (firstCharCode >= 65 && firstCharCode <= 90) || + (firstCharCode >= 97 && firstCharCode <= 122) + } + return false +} + +/** + * Sorts routes based off of whether the shortName begins with an alphabetic + * character. Routes with shortn that do start with an alphabetic character will + * be prioritized over those that don't. + */ +function alphabeticShortNameComparator (a, b) { + const aStartsWithAlphabeticCharacter = startsWithAlphabeticCharacter( + a.shortName + ) + const bStartsWithAlphabeticCharacter = startsWithAlphabeticCharacter( + b.shortName + ) + + if (aStartsWithAlphabeticCharacter && bStartsWithAlphabeticCharacter) { + // both start with an alphabetic character, return equivalence + return 0 + } + // a does start with an alphabetic character, but b does not. Prioritize a + if (aStartsWithAlphabeticCharacter) return -1 + // b does start with an alphabetic character, but a does not. Prioritize b + if (bStartsWithAlphabeticCharacter) return 1 + // neither route has a shortName that starts with an alphabetic character. + // Return equivalence return 0 } +/** + * Checks whether an appropriate comparison of numeric values can be made for + * sorting purposes. If both values are not valid numbers according to the + * isNaN check, then this function returns undefined which indicates that a + * secondary sorting criteria should be used instead. If one value is valid and + * the other is not, then the valid value will be given sorting priority. If + * both values are valid numbers, the difference is obtained as the sort value. + * + * An optional argument can be provided which will be used to obtain the + * comparison value from the comparison function arguments. + * + * IMPORTANT: the comparison values must be numeric values or at least be + * attempted to be converted to numeric values! If one of the arguments is + * something crazy like an empty string, unexpected behavior will occur because + * JavaScript. + * + * @param {function} [objGetterFn] An optional function to obtain the + * comparison value from the comparator function arguments + */ +function makeNumericValueComparator (objGetterFn) { + return (a, b) => { + const { aVal, bVal } = getSortValues(objGetterFn, a, b) + // if both values aren't valid numbers, use the next sort criteria + if (isNaN(aVal) && isNaN(bVal)) return 0 + // b is a valid number, b gets priority + if (isNaN(aVal)) return 1 + // a is a valid number, a gets priority + if (isNaN(bVal)) return -1 + // a and b are valid numbers, return the sort value + return aVal - bVal + } +} + +/** + * Create a comparator function that compares string values. The comparison + * values feed to the sort comparator function are assumed to be objects that + * will have either undefined, null or string values at the given key. If one + * object has undefined, null or an empty string, but the other does have a + * string with length > 0, then that string will get priority. + * + * @param {function} [objGetterFn] An optional function to obtain the + * comparison value from the comparator function arguments + */ +function makeStringValueComparator (objGetterFn) { + return (a, b) => { + const { aVal, bVal } = getSortValues(objGetterFn, a, b) + // both a and b are uncomparable strings, return equivalent value + if (!aVal && !bVal) return 0 + // a is not a comparable string, b gets priority + if (!aVal) return 1 + // b is not a comparable string, a gets priority + if (!bVal) return -1 + // a and b are comparable strings, return the sort value + if (aVal < bVal) return -1 + if (aVal > bVal) return 1 + return 0 + } +} + +/** + * OpenTripPlanner sets the routeSortOrder to -999 by default. So, if that value + * is encountered, assume that it actually means that the routeSortOrder is not + * set in the GTFS. + * + * See https://github.com/opentripplanner/OpenTripPlanner/issues/2938 + * Also see https://github.com/opentripplanner/otp-react-redux/issues/122 + */ +function getRouteSortOrderValue (val) { + return val === -999 ? undefined : val +} + +/** + * Create a multi-criteria sort comparator function composed of other sort + * comparator functions. Each comparator function will be ran in the order given + * until a non-zero comparison value is obtained which is then immediately + * returned. If all comparison functions return equivalance, then the values + * are assumed to be equivalent. + */ +function makeMultiCriteriaSort (...criteria) { + return (a, b) => { + for (let i = 0; i < criteria.length; i++) { + const curCriteriaComparatorValue = criteria[i](a, b) + // if the comparison objects are not equivalent, return the value obtained + // in this current criteria comparison + if (curCriteriaComparatorValue !== 0) { + return curCriteriaComparatorValue + } + } + return 0 + } +} + +/** + * Compares routes for the purposes of sorting and displaying in a user + * interface. Due to GTFS feeds having varying levels of data quality, a multi- + * criteria sort is needed to account for various differences. The criteria + * included here are each applied to the routes in the order listed. If a given + * sort criterion yields equivalence (e.g., two routes have the short name + * "20"), the comparator falls back onto the next sort criterion (e.g., long + * name). If desired, the criteria of sorting based off of integer shortName can + * be disabled. The sort operates on the following values (in order): + * + * 1. sortOrder. Routes that do not have a valid sortOrder will be placed + * beneath those that do. + * 2. route type (OTP mode). See routeTypeComparator code for prioritization of + * route types. + * 3. shortNames that begin with alphabetic characters. shortNames that do not + * start with alphabetic characters will be place beneath those that do. + * 4. shortName as integer. shortNames that cannot be parsed as integers will + * be placed beneath those that are valid. + * 5. shortName as string. Routes without shortNames will be placed beneath + * those with shortNames. + * 6. longName as string. + */ +export const routeComparator = makeMultiCriteriaSort( + makeNumericValueComparator(obj => getRouteSortOrderValue(obj.sortOrder)), + routeTypeComparator, + alphabeticShortNameComparator, + makeNumericValueComparator(obj => parseInt(obj.shortName)), + makeStringValueComparator(obj => obj.shortName), + makeStringValueComparator(obj => obj.longName) +) + /* Returns an interpolated lat-lon at a specified distance along a leg */ export function legLocationAtDistance (leg, distance) {