Enabled verify all shapes and save

[calidion, 2010-04-06 03:09:45]

1.enabled verify all shapes and save

[calidion, 2010-04-06 07:56:00]

server: http://211.100.227.25:8765/

2010/4/6 <calidion@gmail.com>

> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>
> Message:
> 1.enabled verify all shapes and save
>
>
>
> Please review this at http://codereview.appspot.com/868043/show
>
> Affected files:
>  M     gtfsscheduleviewer/files/index.html
>  M     gtfsscheduleviewer/files/style.css
>  M     gtfsscheduleviewer/files/transit_editor.js
>  M     schedule_editor.py
>
>
>

[baiming, 2010-04-06 07:57:15]

thank you for the update. will start reviewing it soon.

2010/4/6 李白，字一日 <calidion@gmail.com>

> server: http://211.100.227.25:8765/
>
> 2010/4/6 <calidion@gmail.com>
>
> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>>
>> Message:
>> 1.enabled verify all shapes and save
>>
>>
>>
>> Please review this at http://codereview.appspot.com/868043/show
>>
>> Affected files:
>>  M     gtfsscheduleviewer/files/index.html
>>  M     gtfsscheduleviewer/files/style.css
>>  M     gtfsscheduleviewer/files/transit_editor.js
>>  M     schedule_editor.py
>>
>>
>>
>


-- 
Ming Bai 白明
Software Engineer
Google Inc.
Tel(Office): 86 (10) 6250-3361
Tel(Cell): +86-159-0153-4908
Email: baiming@google.com

[baiming, 2010-04-06 11:02:29]

I haven't looked at the part of direction retrying. These comments are mostly
for the shape generating.

http://codereview.appspot.com/868043/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/868043/diff/1/2#newcode34
gtfsscheduleviewer/files/transit_editor.js:34: var MAX_TIME = 20;
DIRECTION_MAX_RETRIES

http://codereview.appspot.com/868043/diff/1/2#newcode40
gtfsscheduleviewer/files/transit_editor.js:40: var INTERVAL = 30000;
DIRECTION_INTERVAL

http://codereview.appspot.com/868043/diff/1/2#newcode450
gtfsscheduleviewer/files/transit_editor.js:450: Fetcher.prototype.isShapeExist =
function(shapeId, self, callback) {
Rename it to doesShapeExist or isShapeExisting or hasShape.

http://codereview.appspot.com/868043/diff/1/2#newcode895
gtfsscheduleviewer/files/transit_editor.js:895: * Help function to generate all
shapes of items
Mention in the jsdoc that each object in items has "generateAllShapes" as member
function.

http://codereview.appspot.com/868043/diff/1/2#newcode902
gtfsscheduleviewer/files/transit_editor.js:902: var onItemGen = function() {
s/onItemGen/generateForNext

http://codereview.appspot.com/868043/diff/1/2#newcode903
gtfsscheduleviewer/files/transit_editor.js:903: var item = items[idx];
Why not comparing idx to items.length?

The current way you are using can lead to an unexpected break when encountering
a null item in items.

So I'd prefer to change it to:
if (idx == items.length) {
  if (callback) {
    callback();
  }
  return;
}

if (item) {
  item.generateAllShapes(generateForNext);
  idx++;
}

http://codereview.appspot.com/868043/diff/1/2#newcode917
gtfsscheduleviewer/files/transit_editor.js:917: * Generate All shapes of the
object
Generate route shapes for all the agencies of the manager.

http://codereview.appspot.com/868043/diff/1/2#newcode934
gtfsscheduleviewer/files/transit_editor.js:934: Manager.prototype.getAgency =
function(callback) {
It should be getAgencies.

http://codereview.appspot.com/868043/diff/1/2#newcode1310
gtfsscheduleviewer/files/transit_editor.js:1310: var shapeGenerate = function()
{
s/shapeGenerate/generateNextShape

http://codereview.appspot.com/868043/diff/1/2#newcode1311
gtfsscheduleviewer/files/transit_editor.js:1311: var shapeId = shapeIds[idx];
I totally forgot why we need a separate shapeIdx array. Ain't the keys in shapes
fitting what you need?

http://codereview.appspot.com/868043/diff/1/2#newcode1312
gtfsscheduleviewer/files/transit_editor.js:1312: if (!shapeId) {
Compare idx to shapeIds.length instead.

http://codereview.appspot.com/868043/diff/1/2#newcode1316
gtfsscheduleviewer/files/transit_editor.js:1316: var trips =
self.shapes[shapeId];
Shouldn't we group the trips by pattern, instead of shape, for the route?
Oh, it seems that for now Route hasn't yet been aware of the concept of Pattern,
so it's fine to keep it as is. But do you have any plans to change that in
future?

And rename "shapes" to "tripsByShape".

http://codereview.appspot.com/868043/diff/1/2#newcode1318
gtfsscheduleviewer/files/transit_editor.js:1318: if (!shapes[shapeId]) {
Please add comment that in which case shapes[shapeId] is not existing.

http://codereview.appspot.com/868043/diff/1/2#newcode1318
gtfsscheduleviewer/files/transit_editor.js:1318: if (!shapes[shapeId]) {
Damn! It is the global "shapes".
It is really confusing to see such a variable with a common name which I can't
find where it is defined. And it is very error-prone, look, you override the
global "trips" on line 1316.

We'd better to add some special marks to these global data variables. 

Two options here:
1) (preferred) add a "data" namespace for them. (data.shapes, data.trips, etc)
2) global variables start with an underscore (_shapes, _trips, etc)

http://codereview.appspot.com/868043/diff/1/2#newcode1320
gtfsscheduleviewer/files/transit_editor.js:1320: if (!data[0]) {
Please explain in comment what data[0] means (whether the generated shape is
already existing), and what type it is (a bool value).

http://codereview.appspot.com/868043/diff/1/2#newcode1323
gtfsscheduleviewer/files/transit_editor.js:1323: trip.getStoptimes(function() {
Line 1323~1328 looks like a member function of Trip. 

Trip.prototype.generateAndSaveShape = function(callback) {
  var self = this;
  this.getStoptimes(function() {
    self.parseShapes([]);
    self.verify(function() {
      self.shape.save(callback);
    });
  });
};

http://codereview.appspot.com/868043/diff/1/2#newcode1324
gtfsscheduleviewer/files/transit_editor.js:1324: trip.parseShapes([]);
Why do we need to pass an empty array to parseShapes?

http://codereview.appspot.com/868043/diff/1/2#newcode1325
gtfsscheduleviewer/files/transit_editor.js:1325: trip.verify(function() {
Please rename "verify" to "generateShape" to keep it consistent with UI.

http://codereview.appspot.com/868043/diff/1/2#newcode1330
gtfsscheduleviewer/files/transit_editor.js:1330: shapeGenerate();
Ugh.. too many "shapeGenerate()" here. We should do something.

Add comment for explaining what the else is. i.e,

else {
  // Skip to next if the shape has been generated before.
  generateNextShape();
}

http://codereview.appspot.com/868043/diff/1/2#newcode1332
gtfsscheduleviewer/files/transit_editor.js:1332: });
Can you add a inline comment "  // End of self.fetcher.isShapeExist()" to make
it more readable, since here we got too many clauses.

});  // End of self.fetcher.isShapeExist()

http://codereview.appspot.com/868043/diff/1/2#newcode1334
gtfsscheduleviewer/files/transit_editor.js:1334: shapeGenerate();
Add comment.
// Skip to next if the shape has been generated on server but not loaded to
client.

http://codereview.appspot.com/868043/diff/1/2#newcode1337
gtfsscheduleviewer/files/transit_editor.js:1337: shapeGenerate();
Add comment:
// Start generating.

http://codereview.appspot.com/868043/diff/1/2#newcode1677
gtfsscheduleviewer/files/transit_editor.js:1677: var hintNode = $('gnote');
Is toolbar.hint still in use or not?
Should we delete its creation or keep it for future use?

http://codereview.appspot.com/868043/diff/1/2#newcode1697
gtfsscheduleviewer/files/transit_editor.js:1697: if (show) leg.show();
Break it into two lines for readability.

[calidion, 2010-04-07 08:06:14]

1.fixed a bug on getDirPolyline, move all variables into the function
2.removed on save from trip
3.added save all to global

http://codereview.appspot.com/868043/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/868043/diff/1/2#newcode34
gtfsscheduleviewer/files/transit_editor.js:34: var MAX_TIME = 20;
On 2010/04/06 11:02:32, baiming wrote:
> DIRECTION_MAX_RETRIES

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode40
gtfsscheduleviewer/files/transit_editor.js:40: var INTERVAL = 30000;
On 2010/04/06 11:02:32, baiming wrote:
> DIRECTION_INTERVAL

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode450
gtfsscheduleviewer/files/transit_editor.js:450: Fetcher.prototype.isShapeExist =
function(shapeId, self, callback) {
On 2010/04/06 11:02:32, baiming wrote:
> Rename it to doesShapeExist or isShapeExisting or hasShape.

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode895
gtfsscheduleviewer/files/transit_editor.js:895: * Help function to generate all
shapes of items
On 2010/04/06 11:02:32, baiming wrote:
> Mention in the jsdoc that each object in items has "generateAllShapes" as
member
> function.

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode902
gtfsscheduleviewer/files/transit_editor.js:902: var onItemGen = function() {
On 2010/04/06 11:02:32, baiming wrote:
> s/onItemGen/generateForNext

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode903
gtfsscheduleviewer/files/transit_editor.js:903: var item = items[idx];
On 2010/04/06 11:02:32, baiming wrote:
> Why not comparing idx to items.length?
> 
> The current way you are using can lead to an unexpected break when
encountering
> a null item in items.
> 
> So I'd prefer to change it to:
> if (idx == items.length) {
>   if (callback) {
>     callback();
>   }
>   return;
> }
> 
> if (item) {
>   item.generateAllShapes(generateForNext);
>   idx++;
> }

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode917
gtfsscheduleviewer/files/transit_editor.js:917: * Generate All shapes of the
object
On 2010/04/06 11:02:32, baiming wrote:
> Generate route shapes for all the agencies of the manager.

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode934
gtfsscheduleviewer/files/transit_editor.js:934: Manager.prototype.getAgency =
function(callback) {
On 2010/04/06 11:02:32, baiming wrote:
> It should be getAgencies.

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1310
gtfsscheduleviewer/files/transit_editor.js:1310: var shapeGenerate = function()
{
On 2010/04/06 11:02:32, baiming wrote:
> s/shapeGenerate/generateNextShape

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1311
gtfsscheduleviewer/files/transit_editor.js:1311: var shapeId = shapeIds[idx];
On 2010/04/06 11:02:32, baiming wrote:
> I totally forgot why we need a separate shapeIdx array. Ain't the keys in
shapes
> fitting what you need?

If you really understand this update, you should know the needs of the shapeids.

http://codereview.appspot.com/868043/diff/1/2#newcode1312
gtfsscheduleviewer/files/transit_editor.js:1312: if (!shapeId) {
On 2010/04/06 11:02:32, baiming wrote:
> Compare idx to shapeIds.length instead.

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1316
gtfsscheduleviewer/files/transit_editor.js:1316: var trips =
self.shapes[shapeId];
On 2010/04/06 11:02:32, baiming wrote:
> Shouldn't we group the trips by pattern, instead of shape, for the route?
> Oh, it seems that for now Route hasn't yet been aware of the concept of
Pattern,
> so it's fine to keep it as is. But do you have any plans to change that in
> future?
> 
> And rename "shapes" to "tripsByShape".

We needn't,  all shape must be generated because different shapes must be in
different patterns

http://codereview.appspot.com/868043/diff/1/2#newcode1318
gtfsscheduleviewer/files/transit_editor.js:1318: if (!shapes[shapeId]) {
On 2010/04/06 11:02:32, baiming wrote:
> Please add comment that in which case shapes[shapeId] is not existing.

see the comments where the shapes defined.

http://codereview.appspot.com/868043/diff/1/2#newcode1318
gtfsscheduleviewer/files/transit_editor.js:1318: if (!shapes[shapeId]) {
On 2010/04/06 11:02:32, baiming wrote:
> Damn! It is the global "shapes".
> It is really confusing to see such a variable with a common name which I can't
> find where it is defined. And it is very error-prone, look, you override the
> global "trips" on line 1316.
> 
> We'd better to add some special marks to these global data variables. 
> 
> Two options here:
> 1) (preferred) add a "data" namespace for them. (data.shapes, data.trips, etc)
> 2) global variables start with an underscore (_shapes, _trips, etc)

there are trips, stops, shapes and so on. if it is a big problem, I think we can
avoid it on previous reviews.
and it seems that `data` is still not a good name, where 'data' is widely used
in private scopes.

http://codereview.appspot.com/868043/diff/1/2#newcode1320
gtfsscheduleviewer/files/transit_editor.js:1320: if (!data[0]) {
On 2010/04/06 11:02:32, baiming wrote:
> Please explain in comment what data[0] means (whether the generated shape is
> already existing), and what type it is (a bool value).

added comments in doesShapeExist

http://codereview.appspot.com/868043/diff/1/2#newcode1323
gtfsscheduleviewer/files/transit_editor.js:1323: trip.getStoptimes(function() {
On 2010/04/06 11:02:32, baiming wrote:
> Line 1323~1328 looks like a member function of Trip. 
> 
> Trip.prototype.generateAndSaveShape = function(callback) {
>   var self = this;
>   this.getStoptimes(function() {
>     self.parseShapes([]);
>     self.verify(function() {
>       self.shape.save(callback);
>     });
>   });
> };
> 

it is not needed in any where else now and only used for generating all.

http://codereview.appspot.com/868043/diff/1/2#newcode1324
gtfsscheduleviewer/files/transit_editor.js:1324: trip.parseShapes([]);
On 2010/04/06 11:02:32, baiming wrote:
> Why do we need to pass an empty array to parseShapes? 

Please read more on how the shape is initialized.

http://codereview.appspot.com/868043/diff/1/2#newcode1325
gtfsscheduleviewer/files/transit_editor.js:1325: trip.verify(function() {
On 2010/04/06 11:02:32, baiming wrote:
> Please rename "verify" to "generateShape" to keep it consistent with UI.

Prefer to keep where Legs also have function verify

http://codereview.appspot.com/868043/diff/1/2#newcode1330
gtfsscheduleviewer/files/transit_editor.js:1330: shapeGenerate();
On 2010/04/06 11:02:32, baiming wrote:
> Ugh.. too many "shapeGenerate()" here. We should do something.
> 
> Add comment for explaining what the else is. i.e,
> 
> else {
>   // Skip to next if the shape has been generated before.
>   generateNextShape();
> }

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1332
gtfsscheduleviewer/files/transit_editor.js:1332: });
On 2010/04/06 11:02:32, baiming wrote:
> Can you add a inline comment "  // End of self.fetcher.isShapeExist()" to make
> it more readable, since here we got too many clauses.
> 
> });  // End of self.fetcher.isShapeExist()

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1334
gtfsscheduleviewer/files/transit_editor.js:1334: shapeGenerate();
On 2010/04/06 11:02:32, baiming wrote:
> Add comment.
> // Skip to next if the shape has been generated on server but not loaded to
> client.

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1337
gtfsscheduleviewer/files/transit_editor.js:1337: shapeGenerate();
On 2010/04/06 11:02:32, baiming wrote:
> Add comment:
> // Start generating.

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1677
gtfsscheduleviewer/files/transit_editor.js:1677: var hintNode = $('gnote');
On 2010/04/06 11:02:32, baiming wrote:
> Is toolbar.hint still in use or not?
> Should we delete its creation or keep it for future use?

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1697
gtfsscheduleviewer/files/transit_editor.js:1697: if (show) leg.show();
On 2010/04/06 11:02:32, baiming wrote:
> Break it into two lines for readability. 

Done.

[baiming, 2010-04-07 09:17:12]

Thank you for fixing things I mentioned!

Just a few minor comments, otherwise it looks good to me.

http://codereview.appspot.com/868043/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/868043/diff/1/2#newcode1311
gtfsscheduleviewer/files/transit_editor.js:1311: var shapeId = shapeIds[idx];
On 2010/04/07 08:06:15, calidion wrote:
> On 2010/04/06 11:02:32, baiming wrote:
> > I totally forgot why we need a separate shapeIdx array. Ain't the keys in
> shapes
> > fitting what you need?
> 
> If you really understand this update, you should know the needs of the
shapeids.
> 

Yep, I see.
Forget my stupid question...

http://codereview.appspot.com/868043/diff/1/2#newcode1318
gtfsscheduleviewer/files/transit_editor.js:1318: if (!shapes[shapeId]) {
On 2010/04/07 08:06:15, calidion wrote:
> On 2010/04/06 11:02:32, baiming wrote:
> > Damn! It is the global "shapes".
> > It is really confusing to see such a variable with a common name which I
can't
> > find where it is defined. And it is very error-prone, look, you override the
> > global "trips" on line 1316.
> > 
> > We'd better to add some special marks to these global data variables. 
> > 
> > Two options here:
> > 1) (preferred) add a "data" namespace for them. (data.shapes, data.trips,
etc)
> > 2) global variables start with an underscore (_shapes, _trips, etc)
> 
> there are trips, stops, shapes and so on. if it is a big problem, I think we
can
> avoid it on previous reviews.
> and it seems that `data` is still not a good name, where 'data' is widely used
> in private scopes. 

Right, data is not a good namespace. Hmm, but now it does become a serious
problem. Please contribute some ideas solving this, another namespace? or using
the underscore?

http://codereview.appspot.com/868043/diff/1/2#newcode1324
gtfsscheduleviewer/files/transit_editor.js:1324: trip.parseShapes([]);
On 2010/04/07 08:06:15, calidion wrote:
> On 2010/04/06 11:02:32, baiming wrote:
> > Why do we need to pass an empty array to parseShapes? 
> 
> Please read more on how the shape is initialized.

Can you just add comments here to explain why? People are not as familiar as you
with the your code, and may not want to read too much.

http://codereview.appspot.com/868043/diff/1/2#newcode1325
gtfsscheduleviewer/files/transit_editor.js:1325: trip.verify(function() {
On 2010/04/07 08:06:15, calidion wrote:
> On 2010/04/06 11:02:32, baiming wrote:
> > Please rename "verify" to "generateShape" to keep it consistent with UI.
> 
> Prefer to keep where Legs also have function verify
> 

Shouldn't we change it for Leg as well?

http://codereview.appspot.com/868043/diff/1/2#newcode1330
gtfsscheduleviewer/files/transit_editor.js:1330: shapeGenerate();
On 2010/04/07 08:06:15, calidion wrote:
> On 2010/04/06 11:02:32, baiming wrote:
> > Ugh.. too many "shapeGenerate()" here. We should do something.
> > 
> > Add comment for explaining what the else is. i.e,
> > 
> > else {
> >   // Skip to next if the shape has been generated before.
> >   generateNextShape();
> > }
> 
> Done.

Ah, thank you for the comments!

[calidion, 2010-04-08 02:44:03]

1.fixed a bug on saving shapes
2.update on comments.

http://codereview.appspot.com/868043/diff/1/2
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/868043/diff/1/2#newcode1318
gtfsscheduleviewer/files/transit_editor.js:1318: if (!shapes[shapeId]) {
On 2010/04/07 09:17:13, baiming wrote:
> On 2010/04/07 08:06:15, calidion wrote:
> > On 2010/04/06 11:02:32, baiming wrote:
> > > Damn! It is the global "shapes".
> > > It is really confusing to see such a variable with a common name which I
> can't
> > > find where it is defined. And it is very error-prone, look, you override
the
> > > global "trips" on line 1316.
> > > 
> > > We'd better to add some special marks to these global data variables. 
> > > 
> > > Two options here:
> > > 1) (preferred) add a "data" namespace for them. (data.shapes, data.trips,
> etc)
> > > 2) global variables start with an underscore (_shapes, _trips, etc)
> > 
> > there are trips, stops, shapes and so on. if it is a big problem, I think we
> can
> > avoid it on previous reviews.
> > and it seems that `data` is still not a good name, where 'data' is widely
used
> > in private scopes. 
> 
> Right, data is not a good namespace. Hmm, but now it does become a serious
> problem. Please contribute some ideas solving this, another namespace? or
using
> the underscore?

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1324
gtfsscheduleviewer/files/transit_editor.js:1324: trip.parseShapes([]);
On 2010/04/07 09:17:13, baiming wrote:
> On 2010/04/07 08:06:15, calidion wrote:
> > On 2010/04/06 11:02:32, baiming wrote:
> > > Why do we need to pass an empty array to parseShapes? 
> > 
> > Please read more on how the shape is initialized.
> 
> Can you just add comments here to explain why? People are not as familiar as
you
> with the your code, and may not want to read too much.

Done.

http://codereview.appspot.com/868043/diff/1/2#newcode1325
gtfsscheduleviewer/files/transit_editor.js:1325: trip.verify(function() {
On 2010/04/07 09:17:13, baiming wrote:
> On 2010/04/07 08:06:15, calidion wrote:
> > On 2010/04/06 11:02:32, baiming wrote:
> > > Please rename "verify" to "generateShape" to keep it consistent with UI.
> > 
> > Prefer to keep where Legs also have function verify
> > 
> 
> Shouldn't we change it for Leg as well?

1. verify is more technical than generate shape
2. verify is a basic function to get the trip redirected again regardless of the
trip having or not the shape, and the redirected shape doesn't always need to be
the new shape of the trip.
3. A leg is an element that builds the shape, a leg cannot generate a shape.
what it can do is to get it's self redirected, and sacrifice its result to the
trip or shape to build a new shape or something.

[baiming, 2010-04-08 10:06:27]

LGTM for js code.

http://codereview.appspot.com/868043/diff/18002/13002
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/868043/diff/18002/13002#newcode1689
gtfsscheduleviewer/files/transit_editor.js:1689:
stop.toLeg[this.shape.id].setPolyline(leg.clonePolyline(), Leg.Color.MODIFIED);
The line exceeds 80 char limit.

[weiliu, 2010-04-08 10:08:42]

LGTM, please fix a minor issue.

http://codereview.appspot.com/868043/diff/18002/13005
File schedule_editor.py (right):

http://codereview.appspot.com/868043/diff/18002/13005#newcode106
schedule_editor.py:106: None)
Please fix the indent.

[calidion, 2010-04-09 01:27:56]

updated to rev 70

2010/4/8 <weiliu@google.com>

> LGTM, please fix a minor issue.
>
>
> http://codereview.appspot.com/868043/diff/18002/13005
> File schedule_editor.py (right):
>
> http://codereview.appspot.com/868043/diff/18002/13005#newcode106
> schedule_editor.py:106: None)
> Please fix the indent.
>
>
> http://codereview.appspot.com/868043/show
>