using Route GTFS Object

[calidion, 2010-01-05 07:01:34]

using Route GTFS Object to update route list

[baiming, 2010-01-05 08:57:39]

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

http://codereview.appspot.com/183117/diff/1/2#newcode220
gtfsscheduleviewer/files/transit_editor.js:220: function fetchPatterns(routeId)
{
Add comments for this function.

http://codereview.appspot.com/183117/diff/1/2#newcode221
gtfsscheduleviewer/files/transit_editor.js:221: url =
"/json/routepatterns?route=" + encodeURIComponent(routeId) + "&time=" +
parseTimeInput();
Line length exceeds 80 char limit.

http://codereview.appspot.com/183117/diff/1/2#newcode227
gtfsscheduleviewer/files/transit_editor.js:227: function
callbackDisplayPatterns(data, responseCode) {
Add comments for this function.

http://codereview.appspot.com/183117/diff/1/2#newcode231
gtfsscheduleviewer/files/transit_editor.js:231: var div =
document.createElement("div");
Can you rename "div" to a more meaningful word? such as "patternsDiv".

http://codereview.appspot.com/183117/diff/1/2#newcode231
gtfsscheduleviewer/files/transit_editor.js:231: var div =
document.createElement("div");
Why not using $C for short?

http://codereview.appspot.com/183117/diff/1/2#newcode233
gtfsscheduleviewer/files/transit_editor.js:233: div.id = "tripInfo";
Are "trip" and "pattern" the same concept in the context? I see you are using
both of them for naming, which leads to a bit confusing.

http://codereview.appspot.com/183117/diff/1/2#newcode238
gtfsscheduleviewer/files/transit_editor.js:238: patternDiv =
document.createElement("div")
If the patternDiv is only a local variable within the scope of the for loop, use
"var patternDiv = ..." to declare and initialize it.

http://codereview.appspot.com/183117/diff/1/2#newcode238
gtfsscheduleviewer/files/transit_editor.js:238: patternDiv =
document.createElement("div")
$C?

http://codereview.appspot.com/183117/diff/1/2#newcode241
gtfsscheduleviewer/files/transit_editor.js:241: var pat = patterns[i];  //
[patName, patId, len(early trips), trips, len(later trips),
has_non_zero_trip_type]
1) Line length.
2) What does "early/later trips" mean?

http://codereview.appspot.com/183117/diff/1/2#newcode245
gtfsscheduleviewer/files/transit_editor.js:245:
patternDiv.appendChild(document.createTextNode(pat[0]));
$T?

http://codereview.appspot.com/183117/diff/1/2#newcode246
gtfsscheduleviewer/files/transit_editor.js:246:
patternDiv.appendChild(document.createTextNode(", " + (pat[2] + pat[3].length +
pat[4]) + " trips: "));
$T?

http://codereview.appspot.com/183117/diff/1/2#newcode248
gtfsscheduleviewer/files/transit_editor.js:248:
patternDiv.appendChild(document.createTextNode(countToRepeatedDots(pat[2]) + "
"));
Line length.

http://codereview.appspot.com/183117/diff/1/2#newcode248
gtfsscheduleviewer/files/transit_editor.js:248:
patternDiv.appendChild(document.createTextNode(countToRepeatedDots(pat[2]) + "
"));
$T?

http://codereview.appspot.com/183117/diff/1/2#newcode255
gtfsscheduleviewer/files/transit_editor.js:255:
patternDiv.appendChild(document.createTextNode(" "));
$T?

http://codereview.appspot.com/183117/diff/1/2#newcode256
gtfsscheduleviewer/files/transit_editor.js:256: var span =
document.createElement("span");
$C?

http://codereview.appspot.com/183117/diff/1/2#newcode257
gtfsscheduleviewer/files/transit_editor.js:257:
span.appendChild(document.createTextNode(trip[0]));
$T?

http://codereview.appspot.com/183117/diff/1/2#newcode264
gtfsscheduleviewer/files/transit_editor.js:264:
patternDiv.appendChild(document.createTextNode(" " +
countToRepeatedDots(pat[4])));
$T?
Line length.

http://codereview.appspot.com/183117/diff/1/2#newcode266
gtfsscheduleviewer/files/transit_editor.js:266:
patternDiv.appendChild(document.createElement("br"));
$C?

http://codereview.appspot.com/183117/diff/1/2#newcode395
gtfsscheduleviewer/files/transit_editor.js:395: *                            end
of the action
I suggest you use the four-char indentation in the jsDoc that can save space

http://codereview.appspot.com/183117/diff/1/2#newcode405
gtfsscheduleviewer/files/transit_editor.js:405: * @param {Object} self The
pointer to this of the callback
Is "self" a good name for this parameter? I prefer "obj".
It's up to you.

http://codereview.appspot.com/183117/diff/1/2#newcode410
gtfsscheduleviewer/files/transit_editor.js:410: Fetcher.prototype.getRoutes =
function(self, agencyId, callback) {
Either "self" or "callback" is part of the callback function, they should be put
at the end of the parameter list. Invert "self" and "agencyId".

http://codereview.appspot.com/183117/diff/1/2#newcode639
gtfsscheduleviewer/files/transit_editor.js:639: * condition to determine if
routes are fetched.
s/condition/Condition

Please be cautious about wring comments.

http://codereview.appspot.com/183117/diff/1/2#newcode642
gtfsscheduleviewer/files/transit_editor.js:642: this.retrieved = false;
AFAIS, it is used only within the class. Should be private.

For other existing fields of this class (and other classes), you can keep them
as public at the moment. But I hope that, with the code change is progressing,
we figure out which fields are really needed to be public and which are not.

http://codereview.appspot.com/183117/diff/1/2#newcode648
gtfsscheduleviewer/files/transit_editor.js:648: this.curRoute = null;
Ditto.

http://codereview.appspot.com/183117/diff/1/2#newcode656
gtfsscheduleviewer/files/transit_editor.js:656: Agency.prototype.getRoutes =
function(callback){
As far as I see, callback is always showRoutes(), isn't it?
Using too many callbacks causes the logic becoming complicated. And there's no
much necessity to customize the behavior after an Agency object fetches routes.

http://codereview.appspot.com/183117/diff/1/2#newcode662
gtfsscheduleviewer/files/transit_editor.js:662: this.fetcher.getRoutes(this,
this.id, function(data) {
Make the anonymous function a member function of class Agency.  It will get rid
of the "this/self" passing.

http://codereview.appspot.com/183117/diff/1/2#newcode664
gtfsscheduleviewer/files/transit_editor.js:664: var tmp = data[i];
"tmp" is not really a temp variable, isn't it? If it is hard to find proper name
for it, just data[i]. Because there are only two places referring to it.

http://codereview.appspot.com/183117/diff/1/2#newcode695
gtfsscheduleviewer/files/transit_editor.js:695: this.curRoute.reset();
Two-char indentation instead of four-char.

http://codereview.appspot.com/183117/diff/1/2#newcode763
gtfsscheduleviewer/files/transit_editor.js:763: * Make sure the route assigned a
dom node
"Whether it has been attached to a dom node where it can be displayed."

http://codereview.appspot.com/183117/diff/1/2#newcode771
gtfsscheduleviewer/files/transit_editor.js:771: * Route is a class representing
a GTFS route.
Wrong description of the function.

http://codereview.appspot.com/183117/diff/1/2#newcode813
gtfsscheduleviewer/files/transit_editor.js:813: fetchPatterns(this.id);
After the complete refactoring, will fetchPatterns() be called only within the
Route class? If not, where else it would be called?

[calidion, 2010-01-05 09:52:41]

updated

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

http://codereview.appspot.com/183117/diff/1/2#newcode231
gtfsscheduleviewer/files/transit_editor.js:231: var div =
document.createElement("div");
On 2010/01/05 08:57:39, baiming wrote:
> Why not using $C for short?

this two functions are moved from the original function

http://codereview.appspot.com/183117/diff/1/2#newcode405
gtfsscheduleviewer/files/transit_editor.js:405: * @param {Object} self The
pointer to this of the callback
On 2010/01/05 08:57:39, baiming wrote:
> Is "self" a good name for this parameter? I prefer "obj".
> It's up to you.

I think "self" is more evident to point out it is a this pointer to the callback
function

http://codereview.appspot.com/183117/diff/1/2#newcode410
gtfsscheduleviewer/files/transit_editor.js:410: Fetcher.prototype.getRoutes =
function(self, agencyId, callback) {
On 2010/01/05 08:57:39, baiming wrote:
> Either "self" or "callback" is part of the callback function, they should be
put
> at the end of the parameter list. Invert "self" and "agencyId".

Done.

http://codereview.appspot.com/183117/diff/1/2#newcode639
gtfsscheduleviewer/files/transit_editor.js:639: * condition to determine if
routes are fetched.
On 2010/01/05 08:57:39, baiming wrote:
> s/condition/Condition
> 
> Please be cautious about wring comments.

Done.

http://codereview.appspot.com/183117/diff/1/2#newcode642
gtfsscheduleviewer/files/transit_editor.js:642: this.retrieved = false;
On 2010/01/05 08:57:39, baiming wrote:
> AFAIS, it is used only within the class. Should be private.
> 
> For other existing fields of this class (and other classes), you can keep them
> as public at the moment. But I hope that, with the code change is progressing,
> we figure out which fields are really needed to be public and which are not.

Done.

http://codereview.appspot.com/183117/diff/1/2#newcode648
gtfsscheduleviewer/files/transit_editor.js:648: this.curRoute = null;
On 2010/01/05 08:57:39, baiming wrote:
> Ditto.

Done.

http://codereview.appspot.com/183117/diff/1/2#newcode656
gtfsscheduleviewer/files/transit_editor.js:656: Agency.prototype.getRoutes =
function(callback){
On 2010/01/05 08:57:39, baiming wrote:
> As far as I see, callback is always showRoutes(), isn't it?
> Using too many callbacks causes the logic becoming complicated. And there's no
> much necessity to customize the behavior after an Agency object fetches
routes.

after code evolved into editor, the reason will show up, because the
verification need data only, no need to display any thing.

http://codereview.appspot.com/183117/diff/1/2#newcode662
gtfsscheduleviewer/files/transit_editor.js:662: this.fetcher.getRoutes(this,
this.id, function(data) {
On 2010/01/05 08:57:39, baiming wrote:
> Make the anonymous function a member function of class Agency.  It will get
rid
> of the "this/self" passing.

seems not necessary for this snippet, because we can still use this pointer,
self can be removed, and the function can not be removed because we have the
extra parameter callback

http://codereview.appspot.com/183117/diff/1/2#newcode664
gtfsscheduleviewer/files/transit_editor.js:664: var tmp = data[i];
On 2010/01/05 08:57:39, baiming wrote:
> "tmp" is not really a temp variable, isn't it? If it is hard to find proper
name
> for it, just data[i]. Because there are only two places referring to it.

Done.

http://codereview.appspot.com/183117/diff/1/2#newcode695
gtfsscheduleviewer/files/transit_editor.js:695: this.curRoute.reset();
On 2010/01/05 08:57:39, baiming wrote:
> Two-char indentation instead of four-char.

Done.

http://codereview.appspot.com/183117/diff/1/2#newcode763
gtfsscheduleviewer/files/transit_editor.js:763: * Make sure the route assigned a
dom node
On 2010/01/05 08:57:39, baiming wrote:
> "Whether it has been attached to a dom node where it can be displayed."

Done.

http://codereview.appspot.com/183117/diff/1/2#newcode771
gtfsscheduleviewer/files/transit_editor.js:771: * Route is a class representing
a GTFS route.
On 2010/01/05 08:57:39, baiming wrote:
> Wrong description of the function.

Done.

http://codereview.appspot.com/183117/diff/1/2#newcode813
gtfsscheduleviewer/files/transit_editor.js:813: fetchPatterns(this.id);
On 2010/01/05 08:57:39, baiming wrote:
> After the complete refactoring, will fetchPatterns() be called only within the
> Route class? If not, where else it would be called?

it will be removed.

[baiming, 2010-01-05 12:56:22]

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

http://codereview.appspot.com/183117/diff/1/2#newcode662
gtfsscheduleviewer/files/transit_editor.js:662: this.fetcher.getRoutes(this,
this.id, function(data) {
Yes, directly using "this" in the anonymous function is working, but it looks
weired and is error-prone.

I was confused when I looked to it at the first time, just because of the two
"this"s, one is inside the anonymous function and the other is out. Here they
refer to the same thing, but not necessarily, users must spend extra time to
figure it out. So, let's try to avoid using "this" in any non-prototype
function. It is a good practice, at least from the readability's point of view.

As name, what does Agency.prototype.storeRoutes sound like?

On 2010/01/05 09:52:42, calidion wrote:
> On 2010/01/05 08:57:39, baiming wrote:
> > Make the anonymous function a member function of class Agency.  It will get
> rid
> > of the "this/self" passing.
> 
> seems not necessary for this snippet, because we can still use this pointer,
> self can be removed, and the function can not be removed because we have the
> extra parameter callback 
>

http://codereview.appspot.com/183117/diff/1/2#newcode763
gtfsscheduleviewer/files/transit_editor.js:763: * Make sure the route assigned a
dom node
Done?

"Make sure something..." is really not like a variable description, instead it's
more likely to be function description because we only use verbal phrases for
functions.
On 2010/01/05 09:52:42, calidion wrote:
> On 2010/01/05 08:57:39, baiming wrote:
> > "Whether it has been attached to a dom node where it can be displayed."
> 
> Done.

[calidion, 2010-01-05 14:39:20]

it would be much clearer to comment on the newly uploaded side

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

>
> http://codereview.appspot.com/183117/diff/1/2
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
> http://codereview.appspot.com/183117/diff/1/2#newcode662
> gtfsscheduleviewer/files/transit_editor.js:662:
> this.fetcher.getRoutes(this, this.id, function(data) {
> Yes, directly using "this" in the anonymous function is working, but it
> looks weired and is error-prone.
>
> I was confused when I looked to it at the first time, just because of
> the two "this"s, one is inside the anonymous function and the other is
> out. Here they refer to the same thing, but not necessarily, users must
> spend extra time to figure it out. So, let's try to avoid using "this"
> in any non-prototype function. It is a good practice, at least from the
> readability's point of view.
>
> As name, what does Agency.prototype.storeRoutes sound like?
>
>
> On 2010/01/05 09:52:42, calidion wrote:
>
>> On 2010/01/05 08:57:39, baiming wrote:
>> > Make the anonymous function a member function of class Agency.  It
>>
> will get
>
>> rid
>> > of the "this/self" passing.
>>
>
>  seems not necessary for this snippet, because we can still use this
>>
> pointer,
>
>> self can be removed, and the function can not be removed because we
>>
> have the
>
>> extra parameter callback
>>
>
>
> http://codereview.appspot.com/183117/diff/1/2#newcode763
> gtfsscheduleviewer/files/transit_editor.js:763: * Make sure the route
> assigned a dom node
> Done?
>
> "Make sure something..." is really not like a variable description,
> instead it's more likely to be function description because we only use
> verbal phrases for functions.
>
> On 2010/01/05 09:52:42, calidion wrote:
>
>> On 2010/01/05 08:57:39, baiming wrote:
>> > "Whether it has been attached to a dom node where it can be
>>
> displayed."
>
>  Done.
>>
>
> http://codereview.appspot.com/183117
>

[baiming, 2010-01-06 01:07:32]

I just wanna reply closely to what you said, to keep the context.

2010/1/5 李白，字一日 <calidion@gmail.com>

> it would be much clearer to comment on the newly uploaded side
>
> 2010/1/5 <baiming@google.com>
>
>
>> http://codereview.appspot.com/183117/diff/1/2
>> File gtfsscheduleviewer/files/transit_editor.js (right):
>>
>> http://codereview.appspot.com/183117/diff/1/2#newcode662
>> gtfsscheduleviewer/files/transit_editor.js:662:
>> this.fetcher.getRoutes(this, this.id, function(data) {
>> Yes, directly using "this" in the anonymous function is working, but it
>> looks weired and is error-prone.
>>
>> I was confused when I looked to it at the first time, just because of
>> the two "this"s, one is inside the anonymous function and the other is
>> out. Here they refer to the same thing, but not necessarily, users must
>> spend extra time to figure it out. So, let's try to avoid using "this"
>> in any non-prototype function. It is a good practice, at least from the
>> readability's point of view.
>>
>> As name, what does Agency.prototype.storeRoutes sound like?
>>
>>
>> On 2010/01/05 09:52:42, calidion wrote:
>>
>>> On 2010/01/05 08:57:39, baiming wrote:
>>> > Make the anonymous function a member function of class Agency.  It
>>>
>> will get
>>
>>> rid
>>> > of the "this/self" passing.
>>>
>>
>>  seems not necessary for this snippet, because we can still use this
>>>
>> pointer,
>>
>>> self can be removed, and the function can not be removed because we
>>>
>> have the
>>
>>> extra parameter callback
>>>
>>
>>
>> http://codereview.appspot.com/183117/diff/1/2#newcode763
>> gtfsscheduleviewer/files/transit_editor.js:763: * Make sure the route
>> assigned a dom node
>> Done?
>>
>> "Make sure something..." is really not like a variable description,
>> instead it's more likely to be function description because we only use
>> verbal phrases for functions.
>>
>> On 2010/01/05 09:52:42, calidion wrote:
>>
>>> On 2010/01/05 08:57:39, baiming wrote:
>>> > "Whether it has been attached to a dom node where it can be
>>>
>> displayed."
>>
>>  Done.
>>>
>>
>> http://codereview.appspot.com/183117
>>
>
>


-- 
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-06 02:23:34]

updated.

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

http://codereview.appspot.com/183117/diff/1/2#newcode662
gtfsscheduleviewer/files/transit_editor.js:662: this.fetcher.getRoutes(this,
this.id, function(data) {
I think you may need to understand why Fetcher's member function is always bind
a this pointer.
and why should i use a callback to make this process much smoother?

if we add a new member function for the callback, it doesn't  make much
difference to the current code. only to add a new member function which job is
to parse the data retrieved into routes, and it is still of the process of
getting routes for the agency, the callback function will still be needed
because we have another parameter callback to be passed to the member function.
it wouldn't save us anything but add a new function and make the process more
complicated.

with the invocation of the Fetcher.getRoutes, the callback function is
definitely bound with the this pointer passed by getRoutes. and the invoker must
know it before calling Fetcher.getRoutes. so, the use of this inside the
callback is quite acceptable.

As for the name storeRoute, I don't think this process has any relationship with
the storage of the routes. it is a process of member initialization of agency,
not a storing process of the routes.

the two essential parts together make the routes fully initialized for the
agency, each one of them is incomplete. it is unnecessary to separate them into
two.


On 2010/01/05 12:56:22, baiming wrote:
> Yes, directly using "this" in the anonymous function is working, but it looks
> weired and is error-prone.
> 
> I was confused when I looked to it at the first time, just because of the two
> "this"s, one is inside the anonymous function and the other is out. Here they
> refer to the same thing, but not necessarily, users must spend extra time to
> figure it out. So, let's try to avoid using "this" in any non-prototype
> function. It is a good practice, at least from the readability's point of
view.
> 
> As name, what does Agency.prototype.storeRoutes sound like?
> 
> On 2010/01/05 09:52:42, calidion wrote:
> > On 2010/01/05 08:57:39, baiming wrote:
> > > Make the anonymous function a member function of class Agency.  It will
get
> > rid
> > > of the "this/self" passing.
> > 
> > seems not necessary for this snippet, because we can still use this pointer,
> > self can be removed, and the function can not be removed because we have the
> > extra parameter callback 
> > 
> 
>

http://codereview.appspot.com/183117/diff/1/2#newcode763
gtfsscheduleviewer/files/transit_editor.js:763: * Make sure the route assigned a
dom node
On 2010/01/05 12:56:22, baiming wrote:
> Done?
> 
> "Make sure something..." is really not like a variable description, instead
it's
> more likely to be function description because we only use verbal phrases for
> functions.
> On 2010/01/05 09:52:42, calidion wrote:
> > On 2010/01/05 08:57:39, baiming wrote:
> > > "Whether it has been attached to a dom node where it can be displayed."
> > 
> > Done.
> 
> 

Done.

[calidion, 2010-01-06 09:23:41]

updated

[baiming, 2010-01-06 09:32:41]

LGTM.

Thanks for the changes!

http://codereview.appspot.com/183117/diff/1006/1007
File gtfsscheduleviewer/files/transit_editor.js (right):

http://codereview.appspot.com/183117/diff/1006/1007#newcode656
gtfsscheduleviewer/files/transit_editor.js:656: Agency.prototype.getRoutes =
function(callback){
Look at how you are using the callback parameter, in line 582. The callback
function is not a behavior of the caller Manager, but of the agency itself. 
0) obsolete Agency.prototype.getRoutes() 
1) make the anonymous function as a member store() or other names
2) add a function storeAndShow() { store(); showRoutes(); }
3) add a function getRoutesAndShow() {
  if (this.retrieved_) {
    this.storeAndShow();
    return;
  }
   this.fetcher.getRoutes(this.id, this, this.storeAndShow);
}
4) Maybe you will need add some other functions in future:
verify() {...}
storeAndVerify() { this.store(); this.verify(); }
getRoutesAndVerify() {
  if (this.retrieved_) {
    this.verify();
    return;
  }
   this.fetcher.getRoutes(this.id, this, this.storeAndVerify);
}

http://codereview.appspot.com/183117/diff/1006/1007#newcode656
gtfsscheduleviewer/files/transit_editor.js:656: Agency.prototype.getRoutes =
function(callback){
The root of all evil is the callback parameter of getRoutes, which needs to be
passed to another async function Fetcher.prototype.getRoutes.

Let's have a look if it really worth being or should be a callback.

option 1: replace callback with event, if the callback is really should be done
by caller
option 2: let Agency itself handle the whole procedure, from fetching to
handling response

[baiming, 2010-01-06 09:33:54]

On Wed, Jan 6, 2010 at 5:32 PM, <baiming@google.com> wrote:

> LGTM.
>
> Thanks for the changes!
>
>
> http://codereview.appspot.com/183117/diff/1006/1007
>
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
> Ignore the comments below. They are just drafts that I forgot to delete.


> http://codereview.appspot.com/183117/diff/1006/1007#newcode656
>
> gtfsscheduleviewer/files/transit_editor.js:656:
> Agency.prototype.getRoutes = function(callback){
> Look at how you are using the callback parameter, in line 582. The
> callback function is not a behavior of the caller Manager, but of the
> agency itself.
> 0) obsolete Agency.prototype.getRoutes()
> 1) make the anonymous function as a member store() or other names
> 2) add a function storeAndShow() { store(); showRoutes(); }
> 3) add a function getRoutesAndShow() {
>  if (this.retrieved_) {
>    this.storeAndShow();
>    return;
>  }
>   this.fetcher.getRoutes(this.id, this, this.storeAndShow);
> }
> 4) Maybe you will need add some other functions in future:
> verify() {...}
> storeAndVerify() { this.store(); this.verify(); }
> getRoutesAndVerify() {
>  if (this.retrieved_) {
>    this.verify();
>    return;
>  }
>   this.fetcher.getRoutes(this.id, this, this.storeAndVerify);
> }
>
> http://codereview.appspot.com/183117/diff/1006/1007#newcode656
>
> gtfsscheduleviewer/files/transit_editor.js:656:
> Agency.prototype.getRoutes = function(callback){
> The root of all evil is the callback parameter of getRoutes, which needs
> to be passed to another async function Fetcher.prototype.getRoutes.
>
> Let's have a look if it really worth being or should be a callback.
>
> option 1: replace callback with event, if the callback is really should
> be done by caller
> option 2: let Agency itself handle the whole procedure, from fetching to
> handling response
>
>
> http://codereview.appspot.com/183117
>



-- 
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-06 09:37:16]

updated to rev 42

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

> LGTM.
>
> Thanks for the changes!
>
>
> http://codereview.appspot.com/183117/diff/1006/1007
>
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
> http://codereview.appspot.com/183117/diff/1006/1007#newcode656
>
> gtfsscheduleviewer/files/transit_editor.js:656:
> Agency.prototype.getRoutes = function(callback){
> Look at how you are using the callback parameter, in line 582. The
> callback function is not a behavior of the caller Manager, but of the
> agency itself.
> 0) obsolete Agency.prototype.getRoutes()
> 1) make the anonymous function as a member store() or other names
> 2) add a function storeAndShow() { store(); showRoutes(); }
> 3) add a function getRoutesAndShow() {
>  if (this.retrieved_) {
>    this.storeAndShow();
>    return;
>  }
>   this.fetcher.getRoutes(this.id, this, this.storeAndShow);
> }
> 4) Maybe you will need add some other functions in future:
> verify() {...}
> storeAndVerify() { this.store(); this.verify(); }
> getRoutesAndVerify() {
>  if (this.retrieved_) {
>    this.verify();
>    return;
>  }
>   this.fetcher.getRoutes(this.id, this, this.storeAndVerify);
> }
>
> http://codereview.appspot.com/183117/diff/1006/1007#newcode656
>
> gtfsscheduleviewer/files/transit_editor.js:656:
> Agency.prototype.getRoutes = function(callback){
> The root of all evil is the callback parameter of getRoutes, which needs
> to be passed to another async function Fetcher.prototype.getRoutes.
>
> Let's have a look if it really worth being or should be a callback.
>
> option 1: replace callback with event, if the callback is really should
> be done by caller
> option 2: let Agency itself handle the whole procedure, from fetching to
> handling response
>
>
> http://codereview.appspot.com/183117
>