Issue 186205: using GTFS Shape Object and GTFS Leg Object
Patch Set 1 #
Total comments: 65
Patch Set 2 : updated for duplicated stoptime tag #Patch Set 3 : updated #Patch Set 4 : updated #Patch Set 5 : updated #Patch Set 6 : updated #
MessagesTotal messages: 15
using GTFS Shape Object and GTFS Leg Object
Sign in to reply to this message.
http://codereview.appspot.com/186205/diff/1/4 File schedule_editor.py (right): http://codereview.appspot.com/186205/diff/1/4#newcode137 schedule_editor.py:137: """return shape points for a certain shape""" for the given trip id? http://codereview.appspot.com/186205/diff/1/4#newcode143 schedule_editor.py:143: if shape and ('points' in dir(shape)): if shape and 'points' in dir(shape):
Sign in to reply to this message.
and update on bug fixes 2010/1/19 <weiliu@google.com> > > http://codereview.appspot.com/186205/diff/1/4 > File schedule_editor.py (right): > > http://codereview.appspot.com/186205/diff/1/4#newcode137 > schedule_editor.py:137: """return shape points for a certain shape""" > for the given trip id? > > http://codereview.appspot.com/186205/diff/1/4#newcode143 > schedule_editor.py:143: if shape and ('points' in dir(shape)): > if shape and 'points' in dir(shape): > > > http://codereview.appspot.com/186205/show >
Sign in to reply to this message.
LGTM for python code. 2010/1/19 李白，字一日 <calidion@gmail.com> > and update on bug fixes > > 2010/1/19 <weiliu@google.com> > > >> http://codereview.appspot.com/186205/diff/1/4 >> File schedule_editor.py (right): >> >> http://codereview.appspot.com/186205/diff/1/4#newcode137 >> schedule_editor.py:137: """return shape points for a certain shape""" >> for the given trip id? >> >> http://codereview.appspot.com/186205/diff/1/4#newcode143 >> schedule_editor.py:143: if shape and ('points' in dir(shape)): >> if shape and 'points' in dir(shape): >> >> >> http://codereview.appspot.com/186205/show >> > > -- Best Regards, Wei Liu 86-10-62503256(o)
Sign in to reply to this message.
thanks, please someone review javascript code. 2010/1/19 Wei Liu <weiliu@google.com> > LGTM for python code. > > 2010/1/19 李白，字一日 <calidion@gmail.com> > > and update on bug fixes >> >> 2010/1/19 <weiliu@google.com> >> >> >>> http://codereview.appspot.com/186205/diff/1/4 >>> File schedule_editor.py (right): >>> >>> http://codereview.appspot.com/186205/diff/1/4#newcode137 >>> schedule_editor.py:137: """return shape points for a certain shape""" >>> for the given trip id? >>> >>> http://codereview.appspot.com/186205/diff/1/4#newcode143 >>> schedule_editor.py:143: if shape and ('points' in dir(shape)): >>> if shape and 'points' in dir(shape): >>> >>> >>> http://codereview.appspot.com/186205/show >>> >> >> > > > -- > Best Regards, > Wei Liu > 86-10-62503256(o) >
Sign in to reply to this message.
I am looking at it now. 2010/1/19 李白，字一日 <calidion@gmail.com>: > thanks, > please someone review javascript code. > 2010/1/19 Wei Liu <weiliu@google.com> >> >> LGTM for python code. >> >> 2010/1/19 李白，字一日 <calidion@gmail.com> >>> >>> and update on bug fixes >>> >>> 2010/1/19 <weiliu@google.com> >>>> >>>> http://codereview.appspot.com/186205/diff/1/4 >>>> File schedule_editor.py (right): >>>> >>>> http://codereview.appspot.com/186205/diff/1/4#newcode137 >>>> schedule_editor.py:137: """return shape points for a certain shape""" >>>> for the given trip id? >>>> >>>> http://codereview.appspot.com/186205/diff/1/4#newcode143 >>>> schedule_editor.py:143: if shape and ('points' in dir(shape)): >>>> if shape and 'points' in dir(shape): >>>> >>>> http://codereview.appspot.com/186205/show >>> >> >> >> >> -- >> Best Regards, >> Wei Liu >> 86-10-62503256(o) > > -- Ming Bai 白明 Software Engineer Google Inc. Tel(Office): 86 (10) 6250-3361 Tel(Cell): +86-159-0153-4908 Email: baiming@google.com
Sign in to reply to this message.
Hi Wenxin, How is the port application going? Any response? Fei, could you help him with this? Thanks, - Xinxing 2010/1/19 李白，字一日 <calidion@gmail.com> > thanks, > please someone review javascript code. > > 2010/1/19 Wei Liu <weiliu@google.com> > > LGTM for python code. >> >> 2010/1/19 李白，字一日 <calidion@gmail.com> >> >> and update on bug fixes >>> >>> 2010/1/19 <weiliu@google.com> >>> >>> >>>> http://codereview.appspot.com/186205/diff/1/4 >>>> File schedule_editor.py (right): >>>> >>>> http://codereview.appspot.com/186205/diff/1/4#newcode137 >>>> schedule_editor.py:137: """return shape points for a certain shape""" >>>> for the given trip id? >>>> >>>> http://codereview.appspot.com/186205/diff/1/4#newcode143 >>>> schedule_editor.py:143: if shape and ('points' in dir(shape)): >>>> if shape and 'points' in dir(shape): >>>> >>>> >>>> http://codereview.appspot.com/186205/show >>>> >>> >>> >> >> >> -- >> Best Regards, >> Wei Liu >> 86-10-62503256(o) >> > >
Sign in to reply to this message.
http://codereview.appspot.com/186205/diff/1/3 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/186205/diff/1/3#newcode1009 gtfsscheduleviewer/files/transit_editor.js:1009: self.getShapes(function(){ Space between ) and { http://codereview.appspot.com/186205/diff/1/3#newcode1009 gtfsscheduleviewer/files/transit_editor.js:1009: self.getShapes(function(){ Can we use member functions to avoid the embedded callbacks? http://codereview.appspot.com/186205/diff/1/3#newcode1041 gtfsscheduleviewer/files/transit_editor.js:1041: Trip.prototype.resetShapes = function() { If the only job it does is to hide the shapes, please rename it to hideShapes. http://codereview.appspot.com/186205/diff/1/3#newcode1157 gtfsscheduleviewer/files/transit_editor.js:1157: this.data = []; "data" and "points" are the same? Oh, it looks like you didn't use points at all, instead you use data. For me, "points" much more clearly indicates what it contains, than "data". Please remove "data" and keep "points". http://codereview.appspot.com/186205/diff/1/3#newcode1164 gtfsscheduleviewer/files/transit_editor.js:1164: * Get nearest point in points to latlng This function returns the index of the nearest point in the points array, rather than the point object. Please describe it in the comment. http://codereview.appspot.com/186205/diff/1/3#newcode1171 gtfsscheduleviewer/files/transit_editor.js:1171: var pre = 9999999999; minDist? http://codereview.appspot.com/186205/diff/1/3#newcode1172 gtfsscheduleviewer/files/transit_editor.js:1172: var found = points.length - 1; Is points.length - 1 the index we found of the nearest point? Why do you call it "found"? http://codereview.appspot.com/186205/diff/1/3#newcode1175 gtfsscheduleviewer/files/transit_editor.js:1175: var lat = Math.abs(points[i].lat - latlng.lat()); Please use GLatLng.distanceFrom() to compute distance between points. http://codereview.appspot.com/186205/diff/1/3#newcode1194 gtfsscheduleviewer/files/transit_editor.js:1194: if(!toStop.fromLeg) { Space between if and ( http://codereview.appspot.com/186205/diff/1/3#newcode1194 gtfsscheduleviewer/files/transit_editor.js:1194: if(!toStop.fromLeg) { I don't like the implementation to attach the "from" and "to" legs to the stop, which will never be used by the stop itself. As far as I can see, you do this only to make the legs accessible for shape, via stops. Can we let legs directly be members of shape? I think it'd make more sense and be more natural as legs are just children of the shape. Furthermore, shape even doesn't need to own stops as member. So far all the accesses to "this.stops" from shape is to either 1) iterate legs (in hide() and show()) or 2) create legs (in bindStops(), where we can put stops in the parameters) http://codereview.appspot.com/186205/diff/1/3#newcode1197 gtfsscheduleviewer/files/transit_editor.js:1197: if(!fromStop.toLeg) { Ditto. http://codereview.appspot.com/186205/diff/1/3#newcode1201 gtfsscheduleviewer/files/transit_editor.js:1201: if(!leg) { DItto. http://codereview.appspot.com/186205/diff/1/3#newcode1214 gtfsscheduleviewer/files/transit_editor.js:1214: var start = 0; Move this line down to below 1222, to make it as close to where it is used as possible. http://codereview.appspot.com/186205/diff/1/3#newcode1215 gtfsscheduleviewer/files/transit_editor.js:1215: var end = - 1; Ditto. http://codereview.appspot.com/186205/diff/1/3#newcode1217 gtfsscheduleviewer/files/transit_editor.js:1217: if (!this.data.length) { For the leg which don't have shape, I recommend that we create a two-points polyline (stop - stop) for it. Yes, just as you did in show(). Can we move it to here? that we can benefit from reducing the number of checking the existence of this.polyline? http://codereview.appspot.com/186205/diff/1/3#newcode1227 gtfsscheduleviewer/files/transit_editor.js:1227: for (var j = start; j <= end; j++) { Can we replace this loop with slice() method of array? http://codereview.appspot.com/186205/diff/1/3#newcode1229 gtfsscheduleviewer/files/transit_editor.js:1229: // dist += this.data[j].distanceFrom(this.data[j + 1]); Remove the line which is no longer in use. http://codereview.appspot.com/186205/diff/1/3#newcode1233 gtfsscheduleviewer/files/transit_editor.js:1233: var poly = leg.points2Poly(points, color); s/poly/polyline. Since poly can also refers polygon. http://codereview.appspot.com/186205/diff/1/3#newcode1243 gtfsscheduleviewer/files/transit_editor.js:1243: for (var i = 0; i < this.stops.length - 1; i++) { If we change to make legs as member of shape, it'd be simpler: for (var i = 0; i < this.legs.length; ++i) { this.legs[i].show(); } http://codereview.appspot.com/186205/diff/1/3#newcode1252 gtfsscheduleviewer/files/transit_editor.js:1252: for (var i = 0; i < this.stops.length - 1; i++) { Ditto. http://codereview.appspot.com/186205/diff/1/3#newcode1305 gtfsscheduleviewer/files/transit_editor.js:1305: * @type {Object} @enum {string} Here is an example of enum: /** * Enum for tri-state values. * @enum {number} */ project.TriState = { TRUE: 1, FALSE: -1, MAYBE: 0 }; http://codereview.appspot.com/186205/diff/1/3#newcode1307 gtfsscheduleviewer/files/transit_editor.js:1307: Leg.colors = { 1) According to naming style convention, we use EnumNamesLikeThis. 2) Generally use the single form for enum name. So, please s/colors/Color. http://codereview.appspot.com/186205/diff/1/3#newcode1308 gtfsscheduleviewer/files/transit_editor.js:1308: def: '#0000FF', 1) Use SYMBOLIC_CONSTANTS_LIKE_THIS 2) def in not clear enough, please use DEFAULT s/def/DEFAULT. 3) Try to use the shorthand for CSS color, '#0ff' http://codereview.appspot.com/186205/diff/1/3#newcode1309 gtfsscheduleviewer/files/transit_editor.js:1309: red: '#FF0000', RED '#f00' http://codereview.appspot.com/186205/diff/1/3#newcode1310 gtfsscheduleviewer/files/transit_editor.js:1310: saved: '#FF0000', Ditto http://codereview.appspot.com/186205/diff/1/3#newcode1311 gtfsscheduleviewer/files/transit_editor.js:1311: programmed: '#00FF00', Ditto http://codereview.appspot.com/186205/diff/1/3#newcode1312 gtfsscheduleviewer/files/transit_editor.js:1312: original: '#8552a1', Ditto http://codereview.appspot.com/186205/diff/1/3#newcode1313 gtfsscheduleviewer/files/transit_editor.js:1313: modified: '#7fb80e', Ditto http://codereview.appspot.com/186205/diff/1/3#newcode1315 gtfsscheduleviewer/files/transit_editor.js:1315: preDir: '#1d953f' What does these two words mean? http://codereview.appspot.com/186205/diff/1/3#newcode1346 gtfsscheduleviewer/files/transit_editor.js:1346: Leg.prototype.points2Poly = function(points, color) { Rename it to points2Polyline. And here I have a thought that can we just assign the created polyline to the Leg in this method? http://codereview.appspot.com/186205/diff/1/3#newcode1372 gtfsscheduleviewer/files/transit_editor.js:1372: if (!this.polyline) { Rather than checking the existence of this.polyline every time calling show(), how about moving that logic (to create the two-points polyline) into function bindStops?
Sign in to reply to this message.
updated. http://codereview.appspot.com/186205/diff/1/3 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/186205/diff/1/3#newcode1009 gtfsscheduleviewer/files/transit_editor.js:1009: self.getShapes(function(){ On 2010/01/19 07:07:12, baiming wrote: > Space between ) and { Done. http://codereview.appspot.com/186205/diff/1/3#newcode1009 gtfsscheduleviewer/files/transit_editor.js:1009: self.getShapes(function(){ On 2010/01/19 07:07:12, baiming wrote: > Can we use member functions to avoid the embedded callbacks? Seems not. http://codereview.appspot.com/186205/diff/1/3#newcode1041 gtfsscheduleviewer/files/transit_editor.js:1041: Trip.prototype.resetShapes = function() { On 2010/01/19 07:07:12, baiming wrote: > If the only job it does is to hide the shapes, please rename it to hideShapes. resetShapes means shapes status to be reset, not only hide shapes though currently it only hides the shape. http://codereview.appspot.com/186205/diff/1/3#newcode1157 gtfsscheduleviewer/files/transit_editor.js:1157: this.data = []; On 2010/01/19 07:07:12, baiming wrote: > "data" and "points" are the same? > > Oh, it looks like you didn't use points at all, instead you use data. For me, > "points" much more clearly indicates what it contains, than "data". Please > remove "data" and keep "points". points is reserved for further use. data and points is different in format. for ease of use and calculation, we should keep them all. http://codereview.appspot.com/186205/diff/1/3#newcode1164 gtfsscheduleviewer/files/transit_editor.js:1164: * Get nearest point in points to latlng On 2010/01/19 07:07:12, baiming wrote: > This function returns the index of the nearest point in the points array, rather > than the point object. Please describe it in the comment. Done. http://codereview.appspot.com/186205/diff/1/3#newcode1171 gtfsscheduleviewer/files/transit_editor.js:1171: var pre = 9999999999; On 2010/01/19 07:07:12, baiming wrote: > minDist? Done. http://codereview.appspot.com/186205/diff/1/3#newcode1172 gtfsscheduleviewer/files/transit_editor.js:1172: var found = points.length - 1; On 2010/01/19 07:07:12, baiming wrote: > Is points.length - 1 the index we found of the nearest point? Why do you call it > "found"? default found index http://codereview.appspot.com/186205/diff/1/3#newcode1175 gtfsscheduleviewer/files/transit_editor.js:1175: var lat = Math.abs(points[i].lat - latlng.lat()); On 2010/01/19 07:07:12, baiming wrote: > Please use GLatLng.distanceFrom() to compute distance between points. should we create new GLatLng object every time? http://codereview.appspot.com/186205/diff/1/3#newcode1194 gtfsscheduleviewer/files/transit_editor.js:1194: if(!toStop.fromLeg) { On 2010/01/19 07:07:12, baiming wrote: > Space between if and ( Done. http://codereview.appspot.com/186205/diff/1/3#newcode1194 gtfsscheduleviewer/files/transit_editor.js:1194: if(!toStop.fromLeg) { On 2010/01/19 07:07:12, baiming wrote: > I don't like the implementation to attach the "from" and "to" legs to the stop, > which will never be used by the stop itself. As far as I can see, you do this > only to make the legs accessible for shape, via stops. Can we let legs directly > be members of shape? I think it'd make more sense and be more natural as legs > are just children of the shape. > > Furthermore, shape even doesn't need to own stops as member. So far all the > accesses to "this.stops" from shape is to either > 1) iterate legs (in hide() and show()) > or > 2) create legs (in bindStops(), where we can put stops in the parameters) 1) legs are not related to shapes, although they may be initialized with shapes. 2) legs change with their corresponding two stops. without the stops, they will never exist. 3) only the stops know the legs for certain shapes, shapes doesn't know their legs directly. 4) stops change the legs, not the shape. 5) stops are shared by shapes, so the binding of the legs will let the stops changes be notified to legs. http://codereview.appspot.com/186205/diff/1/3#newcode1214 gtfsscheduleviewer/files/transit_editor.js:1214: var start = 0; On 2010/01/19 07:07:12, baiming wrote: > Move this line down to below 1222, to make it as close to where it is used as > possible. Done. http://codereview.appspot.com/186205/diff/1/3#newcode1215 gtfsscheduleviewer/files/transit_editor.js:1215: var end = - 1; On 2010/01/19 07:07:12, baiming wrote: > Ditto. Done. http://codereview.appspot.com/186205/diff/1/3#newcode1227 gtfsscheduleviewer/files/transit_editor.js:1227: for (var j = start; j <= end; j++) { On 2010/01/19 07:07:12, baiming wrote: > Can we replace this loop with slice() method of array? why and how? http://codereview.appspot.com/186205/diff/1/3#newcode1229 gtfsscheduleviewer/files/transit_editor.js:1229: // dist += this.data[j].distanceFrom(this.data[j + 1]); On 2010/01/19 07:07:12, baiming wrote: > Remove the line which is no longer in use. Done. http://codereview.appspot.com/186205/diff/1/3#newcode1243 gtfsscheduleviewer/files/transit_editor.js:1243: for (var i = 0; i < this.stops.length - 1; i++) { On 2010/01/19 07:07:12, baiming wrote: > If we change to make legs as member of shape, it'd be simpler: > for (var i = 0; i < this.legs.length; ++i) { > this.legs[i].show(); > } it aches when we need it to be editable. http://codereview.appspot.com/186205/diff/1/3#newcode1305 gtfsscheduleviewer/files/transit_editor.js:1305: * @type {Object} On 2010/01/19 07:07:12, baiming wrote: > @enum {string} > > Here is an example of enum: > /** > * Enum for tri-state values. > * @enum {number} > */ > project.TriState = { > TRUE: 1, > FALSE: -1, > MAYBE: 0 > }; Done. http://codereview.appspot.com/186205/diff/1/3#newcode1307 gtfsscheduleviewer/files/transit_editor.js:1307: Leg.colors = { On 2010/01/19 07:07:12, baiming wrote: > 1) According to naming style convention, we use EnumNamesLikeThis. > 2) Generally use the single form for enum name. > > So, please s/colors/Color. Done. http://codereview.appspot.com/186205/diff/1/3#newcode1308 gtfsscheduleviewer/files/transit_editor.js:1308: def: '#0000FF', On 2010/01/19 07:07:12, baiming wrote: > 1) Use SYMBOLIC_CONSTANTS_LIKE_THIS > 2) def in not clear enough, please use DEFAULT > > s/def/DEFAULT. > > 3) Try to use the shorthand for CSS color, '#0ff' Done. http://codereview.appspot.com/186205/diff/1/3#newcode1309 gtfsscheduleviewer/files/transit_editor.js:1309: red: '#FF0000', On 2010/01/19 07:07:12, baiming wrote: > RED > '#f00' Done. http://codereview.appspot.com/186205/diff/1/3#newcode1310 gtfsscheduleviewer/files/transit_editor.js:1310: saved: '#FF0000', On 2010/01/19 07:07:12, baiming wrote: > Ditto Done. http://codereview.appspot.com/186205/diff/1/3#newcode1311 gtfsscheduleviewer/files/transit_editor.js:1311: programmed: '#00FF00', On 2010/01/19 07:07:12, baiming wrote: > Ditto Done. http://codereview.appspot.com/186205/diff/1/3#newcode1312 gtfsscheduleviewer/files/transit_editor.js:1312: original: '#8552a1', On 2010/01/19 07:07:12, baiming wrote: > Ditto Done. http://codereview.appspot.com/186205/diff/1/3#newcode1313 gtfsscheduleviewer/files/transit_editor.js:1313: modified: '#7fb80e', On 2010/01/19 07:07:12, baiming wrote: > Ditto Done. http://codereview.appspot.com/186205/diff/1/3#newcode1315 gtfsscheduleviewer/files/transit_editor.js:1315: preDir: '#1d953f' On 2010/01/19 07:07:12, baiming wrote: > What does these two words mean? Done. http://codereview.appspot.com/186205/diff/1/3#newcode1346 gtfsscheduleviewer/files/transit_editor.js:1346: Leg.prototype.points2Poly = function(points, color) { On 2010/01/19 07:07:12, baiming wrote: > Rename it to points2Polyline. > > And here I have a thought that can we just assign the created polyline to the > Leg in this method? No. we shouldn't. http://codereview.appspot.com/186205/diff/1/3#newcode1372 gtfsscheduleviewer/files/transit_editor.js:1372: if (!this.polyline) { On 2010/01/19 07:07:12, baiming wrote: > Rather than checking the existence of this.polyline every time calling show(), > how about moving that logic (to create the two-points polyline) into function > bindStops? Done.
Sign in to reply to this message.
Thanks for the changes. There are still some parts we haven't yet reached agreements, let's discuss them over phone. http://codereview.appspot.com/186205/diff/1/3 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/186205/diff/1/3#newcode1041 gtfsscheduleviewer/files/transit_editor.js:1041: Trip.prototype.resetShapes = function() { On 2010/01/19 07:07:12, baiming wrote: > If the only job it does is to hide the shapes, please rename it to hideShapes. Please elaborate a bit what kind of resetting will be involved in later. http://codereview.appspot.com/186205/diff/1/3#newcode1157 gtfsscheduleviewer/files/transit_editor.js:1157: this.data = []; Can you describe the potential usage a bit more? On 2010/01/19 09:11:10, calidion wrote: > On 2010/01/19 07:07:12, baiming wrote: > > "data" and "points" are the same? > > > > Oh, it looks like you didn't use points at all, instead you use data. For me, > > "points" much more clearly indicates what it contains, than "data". Please > > remove "data" and keep "points". > > points is reserved for further use. > data and points is different in format. > > for ease of use and calculation, we should keep them all. > http://codereview.appspot.com/186205/diff/1/3#newcode1172 gtfsscheduleviewer/files/transit_editor.js:1172: var found = points.length - 1; It does not make sense. Replace it with some other terms such as "lastIndex" On 2010/01/19 09:11:10, calidion wrote: > On 2010/01/19 07:07:12, baiming wrote: > > Is points.length - 1 the index we found of the nearest point? Why do you call > it > > "found"? > default found index > http://codereview.appspot.com/186205/diff/1/3#newcode1175 gtfsscheduleviewer/files/transit_editor.js:1175: var lat = Math.abs(points[i].lat - latlng.lat()); If you are concerned about the efficiency, you can do some cache to avoid duplicated instantiation. Anyway, do not compute the distance in the current way. It is not correct. On 2010/01/19 09:11:10, calidion wrote: > On 2010/01/19 07:07:12, baiming wrote: > > Please use GLatLng.distanceFrom() to compute distance between points. > > should we create new GLatLng object every time? > http://codereview.appspot.com/186205/diff/1/3#newcode1194 gtfsscheduleviewer/files/transit_editor.js:1194: if(!toStop.fromLeg) { I didn't catch your point... Let's talk over phone. On 2010/01/19 09:11:10, calidion wrote: > On 2010/01/19 07:07:12, baiming wrote: > > I don't like the implementation to attach the "from" and "to" legs to the > stop, > > which will never be used by the stop itself. As far as I can see, you do this > > only to make the legs accessible for shape, via stops. Can we let legs > directly > > be members of shape? I think it'd make more sense and be more natural as legs > > are just children of the shape. > > > > Furthermore, shape even doesn't need to own stops as member. So far all the > > accesses to "this.stops" from shape is to either > > 1) iterate legs (in hide() and show()) > > or > > 2) create legs (in bindStops(), where we can put stops in the parameters) > > 1) legs are not related to shapes, although they may be initialized with shapes. > 2) legs change with their corresponding two stops. > without the stops, they will never exist. > 3) only the stops know the legs for certain shapes, shapes doesn't know their > legs directly. > 4) stops change the legs, not the shape. > 5) stops are shared by shapes, so the binding of the legs will let the stops > changes be notified to legs. > http://codereview.appspot.com/186205/diff/1/3#newcode1227 gtfsscheduleviewer/files/transit_editor.js:1227: for (var j = start; j <= end; j++) { points = this.data.slice(start, end + 1); On 2010/01/19 09:11:10, calidion wrote: > On 2010/01/19 07:07:12, baiming wrote: > > Can we replace this loop with slice() method of array? > why and how? >
Sign in to reply to this message.
On Tue, Jan 19, 2010 at 5:55 PM, <baiming@google.com> wrote: > Thanks for the changes. > > There are still some parts we haven't yet reached agreements, let's > discuss them over phone. > > > http://codereview.appspot.com/186205/diff/1/3 > File gtfsscheduleviewer/files/transit_editor.js (right): > > http://codereview.appspot.com/186205/diff/1/3#newcode1041 > gtfsscheduleviewer/files/transit_editor.js:1041: > Trip.prototype.resetShapes = function() { > On 2010/01/19 07:07:12, baiming wrote: >> >> If the only job it does is to hide the shapes, please rename it to > > hideShapes. > > Please elaborate a bit what kind of resetting will be involved in later. > > http://codereview.appspot.com/186205/diff/1/3#newcode1157 > gtfsscheduleviewer/files/transit_editor.js:1157: this.data = []; > Can you describe the potential usage a bit more? > > On 2010/01/19 09:11:10, calidion wrote: >> >> On 2010/01/19 07:07:12, baiming wrote: >> > "data" and "points" are the same? >> > >> > Oh, it looks like you didn't use points at all, instead you use > > data. For me, >> >> > "points" much more clearly indicates what it contains, than "data". > > Please >> >> > remove "data" and keep "points". > >> points is reserved for further use. >> data and points is different in format. > >> for ease of use and calculation, we should keep them all. "metaPoints" or "pointsMetadata" might be a choice for the name of "data". > > > http://codereview.appspot.com/186205/diff/1/3#newcode1172 > gtfsscheduleviewer/files/transit_editor.js:1172: var found = > points.length - 1; > It does not make sense. Replace it with some other terms such as > "lastIndex" > > On 2010/01/19 09:11:10, calidion wrote: >> >> On 2010/01/19 07:07:12, baiming wrote: >> > Is points.length - 1 the index we found of the nearest point? Why do > > you call >> >> it >> > "found"? >> default found index > > > http://codereview.appspot.com/186205/diff/1/3#newcode1175 > gtfsscheduleviewer/files/transit_editor.js:1175: var lat = > Math.abs(points[i].lat - latlng.lat()); > If you are concerned about the efficiency, you can do some cache to > avoid duplicated instantiation. > Anyway, do not compute the distance in the current way. It is not > correct. > > On 2010/01/19 09:11:10, calidion wrote: >> >> On 2010/01/19 07:07:12, baiming wrote: >> > Please use GLatLng.distanceFrom() to compute distance between > > points. > >> should we create new GLatLng object every time? > > > http://codereview.appspot.com/186205/diff/1/3#newcode1194 > gtfsscheduleviewer/files/transit_editor.js:1194: if(!toStop.fromLeg) { > I didn't catch your point... > Let's talk over phone. > > On 2010/01/19 09:11:10, calidion wrote: >> >> On 2010/01/19 07:07:12, baiming wrote: >> > I don't like the implementation to attach the "from" and "to" legs > > to the >> >> stop, >> > which will never be used by the stop itself. As far as I can see, > > you do this >> >> > only to make the legs accessible for shape, via stops. Can we let > > legs >> >> directly >> > be members of shape? I think it'd make more sense and be more > > natural as legs >> >> > are just children of the shape. >> > >> > Furthermore, shape even doesn't need to own stops as member. So far > > all the >> >> > accesses to "this.stops" from shape is to either >> > 1) iterate legs (in hide() and show()) >> > or >> > 2) create legs (in bindStops(), where we can put stops in the > > parameters) > >> 1) legs are not related to shapes, although they may be initialized > > with shapes. >> >> 2) legs change with their corresponding two stops. >> without the stops, they will never exist. >> 3) only the stops know the legs for certain shapes, shapes doesn't > > know their >> >> legs directly. >> 4) stops change the legs, not the shape. >> 5) stops are shared by shapes, so the binding of the legs will let the > > stops >> >> changes be notified to legs. > > > http://codereview.appspot.com/186205/diff/1/3#newcode1227 > gtfsscheduleviewer/files/transit_editor.js:1227: for (var j = start; j > <= end; j++) { > points = this.data.slice(start, end + 1); > > On 2010/01/19 09:11:10, calidion wrote: >> >> On 2010/01/19 07:07:12, baiming wrote: >> > Can we replace this loop with slice() method of array? >> why and how? > > > http://codereview.appspot.com/186205/show > -- Ming Bai 白明 Software Engineer Google Inc. Tel(Office): 86 (10) 6250-3361 Tel(Cell): +86-159-0153-4908 Email: baiming@google.com
Sign in to reply to this message.
Yeah, sounds much clearer than just data. - Xinxing On Tue, Jan 19, 2010 at 6:16 PM, Ming Bai 白明 <baiming@google.com> wrote: > On Tue, Jan 19, 2010 at 5:55 PM, <baiming@google.com> wrote: > > Thanks for the changes. > > > > There are still some parts we haven't yet reached agreements, let's > > discuss them over phone. > > > > > > http://codereview.appspot.com/186205/diff/1/3 > > File gtfsscheduleviewer/files/transit_editor.js (right): > > > > http://codereview.appspot.com/186205/diff/1/3#newcode1041 > > gtfsscheduleviewer/files/transit_editor.js:1041: > > Trip.prototype.resetShapes = function() { > > On 2010/01/19 07:07:12, baiming wrote: > >> > >> If the only job it does is to hide the shapes, please rename it to > > > > hideShapes. > > > > Please elaborate a bit what kind of resetting will be involved in later. > > > > http://codereview.appspot.com/186205/diff/1/3#newcode1157 > > gtfsscheduleviewer/files/transit_editor.js:1157: this.data = []; > > Can you describe the potential usage a bit more? > > > > On 2010/01/19 09:11:10, calidion wrote: > >> > >> On 2010/01/19 07:07:12, baiming wrote: > >> > "data" and "points" are the same? > >> > > >> > Oh, it looks like you didn't use points at all, instead you use > > > > data. For me, > >> > >> > "points" much more clearly indicates what it contains, than "data". > > > > Please > >> > >> > remove "data" and keep "points". > > > >> points is reserved for further use. > >> data and points is different in format. > > > >> for ease of use and calculation, we should keep them all. > > "metaPoints" or "pointsMetadata" might be a choice for the name of "data". > > > > > > http://codereview.appspot.com/186205/diff/1/3#newcode1172 > > gtfsscheduleviewer/files/transit_editor.js:1172: var found = > > points.length - 1; > > It does not make sense. Replace it with some other terms such as > > "lastIndex" > > > > On 2010/01/19 09:11:10, calidion wrote: > >> > >> On 2010/01/19 07:07:12, baiming wrote: > >> > Is points.length - 1 the index we found of the nearest point? Why do > > > > you call > >> > >> it > >> > "found"? > >> default found index > > > > > > http://codereview.appspot.com/186205/diff/1/3#newcode1175 > > gtfsscheduleviewer/files/transit_editor.js:1175: var lat = > > Math.abs(points[i].lat - latlng.lat()); > > If you are concerned about the efficiency, you can do some cache to > > avoid duplicated instantiation. > > Anyway, do not compute the distance in the current way. It is not > > correct. > > > > On 2010/01/19 09:11:10, calidion wrote: > >> > >> On 2010/01/19 07:07:12, baiming wrote: > >> > Please use GLatLng.distanceFrom() to compute distance between > > > > points. > > > >> should we create new GLatLng object every time? > > > > > > http://codereview.appspot.com/186205/diff/1/3#newcode1194 > > gtfsscheduleviewer/files/transit_editor.js:1194: if(!toStop.fromLeg) { > > I didn't catch your point... > > Let's talk over phone. > > > > On 2010/01/19 09:11:10, calidion wrote: > >> > >> On 2010/01/19 07:07:12, baiming wrote: > >> > I don't like the implementation to attach the "from" and "to" legs > > > > to the > >> > >> stop, > >> > which will never be used by the stop itself. As far as I can see, > > > > you do this > >> > >> > only to make the legs accessible for shape, via stops. Can we let > > > > legs > >> > >> directly > >> > be members of shape? I think it'd make more sense and be more > > > > natural as legs > >> > >> > are just children of the shape. > >> > > >> > Furthermore, shape even doesn't need to own stops as member. So far > > > > all the > >> > >> > accesses to "this.stops" from shape is to either > >> > 1) iterate legs (in hide() and show()) > >> > or > >> > 2) create legs (in bindStops(), where we can put stops in the > > > > parameters) > > > >> 1) legs are not related to shapes, although they may be initialized > > > > with shapes. > >> > >> 2) legs change with their corresponding two stops. > >> without the stops, they will never exist. > >> 3) only the stops know the legs for certain shapes, shapes doesn't > > > > know their > >> > >> legs directly. > >> 4) stops change the legs, not the shape. > >> 5) stops are shared by shapes, so the binding of the legs will let the > > > > stops > >> > >> changes be notified to legs. > > > > > > http://codereview.appspot.com/186205/diff/1/3#newcode1227 > > gtfsscheduleviewer/files/transit_editor.js:1227: for (var j = start; j > > <= end; j++) { > > points = this.data.slice(start, end + 1); > > > > On 2010/01/19 09:11:10, calidion wrote: > >> > >> On 2010/01/19 07:07:12, baiming wrote: > >> > Can we replace this loop with slice() method of array? > >> why and how? > > > > > > http://codereview.appspot.com/186205/show > > > > > > -- > Ming Bai 白明 > Software Engineer > Google Inc. > Tel(Office): 86 (10) 6250-3361 > Tel(Cell): +86-159-0153-4908 > Email: baiming@google.com >
Sign in to reply to this message.
updated.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
updated to rev 47 2010/1/19 <baiming@google.com> > LGTM > > > http://codereview.appspot.com/186205/show >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
