start using GTFS Stoptime and Stop object

[calidion, 2010-01-12 07:57:00]

updated to use Stoptime and Stop objects.

[baiming, 2010-01-12 08:18:40]

Hi Wenxin,

Can you also provide the demo link to us? I remembered Liyong had told
you to do that before.

On Tue, Jan 12, 2010 at 3:57 PM,  <calidion@gmail.com> wrote:
> Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
>
> Message:
> updated to use Stoptime and Stop objects.
>
>
>
> Please review this at http://codereview.appspot.com/184083
>
> Affected files:
>  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

[calidion, 2010-01-12 08:25:25]

今天我们部门刚搬家，还需要过几天才能完成。

2010/1/12 Ming Bai 白明 <baiming@google.com>

> Hi Wenxin,
>
> Can you also provide the demo link to us? I remembered Liyong had told
> you to do that before.
>
> On Tue, Jan 12, 2010 at 3:57 PM,  <calidion@gmail.com> wrote:
> > Reviewers: lychen, xan_google.com, weiliu, leio.chen, baiming, qiaojian,
> >
> > Message:
> > updated to use Stoptime and Stop objects.
> >
> >
> >
> > Please review this at http://codereview.appspot.com/184083
> >
> > Affected files:
> >  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-01-12 12:05:54]

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

http://codereview.appspot.com/184083/diff/1/2#newcode158
gtfsscheduleviewer/files/transit_editor.js:158: gtfs.data.stops = {};
Please write comments for this, to include below information:
1) What is this global map used for?
2) How is it internally structured, that maps what values to what type of key?

http://codereview.appspot.com/184083/diff/1/2#newcode1004
gtfsscheduleviewer/files/transit_editor.js:1004: * @param {Object} data JSON
evaled data
Isn't data an array?
Please update its description accordingly.

http://codereview.appspot.com/184083/diff/1/2#newcode1008
gtfsscheduleviewer/files/transit_editor.js:1008: if(!data || !data.length)
return;
Need a space between if and (.

http://codereview.appspot.com/184083/diff/1/2#newcode1010
gtfsscheduleviewer/files/transit_editor.js:1010: var stopTag = false;
stopTag doesn't contain enough information what it is used for. As I understand,
this flag is to indicate whether it is the first time we parse the given shape
that we need to create stops for it. If I understand correct, we can name it
shapeNotParsed.

Or any other names you like, as long as it indicates what it does.

http://codereview.appspot.com/184083/diff/1/2#newcode1033
gtfsscheduleviewer/files/transit_editor.js:1033:
gtfs.data.stops[this.shapeId].push(stop);
Why not using this.stops instead?

http://codereview.appspot.com/184083/diff/1/2#newcode1040
gtfsscheduleviewer/files/transit_editor.js:1040: stoptime.stop =
gtfs.data.stops[this.shapeId][stoptime.sequence - 1];
Ditto.

http://codereview.appspot.com/184083/diff/1/2#newcode1197
gtfsscheduleviewer/files/transit_editor.js:1197: Stop.icons = null;
Add jsDoc comment for it.

http://codereview.appspot.com/184083/diff/1/2#newcode1202
gtfsscheduleviewer/files/transit_editor.js:1202: Stop.prototype.initIcons =
function() {
It shouldn't be a member function because it only initializes the "static" icons
for the Stop class, not for an instance.
Please make it static (Stop.initIcons), and call it in gtfsLoad().

[calidion, 2010-01-13 02:01:57]

我已经更新了。但是appspot.com已经访问不了。

2010/1/12 <baiming@google.com>

>
> http://codereview.appspot.com/184083/diff/1/2
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
> http://codereview.appspot.com/184083/diff/1/2#newcode158
> gtfsscheduleviewer/files/transit_editor.js:158: gtfs.data.stops = {};
> Please write comments for this, to include below information:
> 1) What is this global map used for?
> 2) How is it internally structured, that maps what values to what type
> of key?
>
> http://codereview.appspot.com/184083/diff/1/2#newcode1004
> gtfsscheduleviewer/files/transit_editor.js:1004: * @param {Object} data
> JSON evaled data
> Isn't data an array?
> Please update its description accordingly.
>
> http://codereview.appspot.com/184083/diff/1/2#newcode1008
> gtfsscheduleviewer/files/transit_editor.js:1008: if(!data ||
> !data.length) return;
> Need a space between if and (.
>
> http://codereview.appspot.com/184083/diff/1/2#newcode1010
> gtfsscheduleviewer/files/transit_editor.js:1010: var stopTag = false;
> stopTag doesn't contain enough information what it is used for. As I
> understand, this flag is to indicate whether it is the first time we
> parse the given shape that we need to create stops for it. If I
> understand correct, we can name it shapeNotParsed.
>
> Or any other names you like, as long as it indicates what it does.
>
> http://codereview.appspot.com/184083/diff/1/2#newcode1033
> gtfsscheduleviewer/files/transit_editor.js:1033:
> gtfs.data.stops[this.shapeId].push(stop);
> Why not using this.stops instead?
>
> http://codereview.appspot.com/184083/diff/1/2#newcode1040
> gtfsscheduleviewer/files/transit_editor.js:1040: stoptime.stop =
> gtfs.data.stops[this.shapeId][stoptime.sequence - 1];
> Ditto.
>
> http://codereview.appspot.com/184083/diff/1/2#newcode1197
> gtfsscheduleviewer/files/transit_editor.js:1197: Stop.icons = null;
> Add jsDoc comment for it.
>
> http://codereview.appspot.com/184083/diff/1/2#newcode1202
> gtfsscheduleviewer/files/transit_editor.js:1202:
> Stop.prototype.initIcons = function() {
> It shouldn't be a member function because it only initializes the
> "static" icons for the Stop class, not for an instance.
> Please make it static (Stop.initIcons), and call it in gtfsLoad().
>
>
> http://codereview.appspot.com/184083
>

[calidion, 2010-01-13 02:07:36]

updated

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

http://codereview.appspot.com/184083/diff/1/2#newcode158
gtfsscheduleviewer/files/transit_editor.js:158: gtfs.data.stops = {};
The stops will be shared in shapes.
so we will using this global variable later.
and the import of using shape and leg will make this upload includes too much
changes. so i will delay its use to the next uploads.

On 2010/01/12 12:05:54, baiming wrote:
> Please write comments for this, to include below information:
> 1) What is this global map used for?
> 2) How is it internally structured, that maps what values to what type of key?

http://codereview.appspot.com/184083/diff/1/2#newcode1004
gtfsscheduleviewer/files/transit_editor.js:1004: * @param {Object} data JSON
evaled data
On 2010/01/12 12:05:54, baiming wrote:
> Isn't data an array?
> Please update its description accordingly.

Done.

http://codereview.appspot.com/184083/diff/1/2#newcode1008
gtfsscheduleviewer/files/transit_editor.js:1008: if(!data || !data.length)
return;
On 2010/01/12 12:05:54, baiming wrote:
> Need a space between if and (.

Done.

http://codereview.appspot.com/184083/diff/1/2#newcode1010
gtfsscheduleviewer/files/transit_editor.js:1010: var stopTag = false;
On 2010/01/12 12:05:54, baiming wrote:
> stopTag doesn't contain enough information what it is used for. As I
understand,
> this flag is to indicate whether it is the first time we parse the given shape
> that we need to create stops for it. If I understand correct, we can name it
> shapeNotParsed.
> 
> Or any other names you like, as long as it indicates what it does.

Done.

http://codereview.appspot.com/184083/diff/1/2#newcode1033
gtfsscheduleviewer/files/transit_editor.js:1033:
gtfs.data.stops[this.shapeId].push(stop);
as previously explained.
On 2010/01/12 12:05:54, baiming wrote:
> Why not using this.stops instead?

http://codereview.appspot.com/184083/diff/1/2#newcode1040
gtfsscheduleviewer/files/transit_editor.js:1040: stoptime.stop =
gtfs.data.stops[this.shapeId][stoptime.sequence - 1];
On 2010/01/12 12:05:54, baiming wrote:
> Ditto.

Done.

http://codereview.appspot.com/184083/diff/1/2#newcode1197
gtfsscheduleviewer/files/transit_editor.js:1197: Stop.icons = null;
On 2010/01/12 12:05:54, baiming wrote:
> Add jsDoc comment for it.

Done.

http://codereview.appspot.com/184083/diff/1/2#newcode1202
gtfsscheduleviewer/files/transit_editor.js:1202: Stop.prototype.initIcons =
function() {
On 2010/01/12 12:05:54, baiming wrote:
> It shouldn't be a member function because it only initializes the "static"
icons
> for the Stop class, not for an instance.
> Please make it static (Stop.initIcons), and call it in gtfsLoad().
> 

Done.

[calidion, 2010-01-13 05:22:49]

updated

[baiming, 2010-01-13 05:26:03]

LGTM (only for JS code).

[weiliu, 2010-01-13 05:31:28]

LGTM for python code.

[calidion, 2010-01-13 05:38:50]

thanks,
updated to rev 45.

2010/1/13 <weiliu@google.com>

> LGTM for python code.
>
>
> http://codereview.appspot.com/184083
>