Issue 181121: added Map Tool and merged functions into it.

baiming

http://codereview.appspot.com/181121/diff/1/2 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/181121/diff/1/2#newcode943 gtfsscheduleviewer/files/transit_editor.js:943: * Add @param for id http://codereview.appspot.com/181121/diff/1/2#newcode946 gtfsscheduleviewer/files/transit_editor.js:946: this.id = ...

14 years, 3 months ago (2010-01-04 08:41:01 UTC) #2

calidion

updated on comments. http://codereview.appspot.com/181121/diff/1/2 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/181121/diff/1/2#newcode943 gtfsscheduleviewer/files/transit_editor.js:943: * On 2010/01/04 08:41:02, baiming wrote: ...

14 years, 3 months ago (2010-01-04 10:09:41 UTC) #3

updated on comments.

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

http://codereview.appspot.com/181121/diff/1/2#newcode943
gtfsscheduleviewer/files/transit_editor.js:943: *
On 2010/01/04 08:41:02, baiming wrote:
> Add @param for id

Done.

http://codereview.appspot.com/181121/diff/1/2#newcode946
gtfsscheduleviewer/files/transit_editor.js:946: this.id = id;
On 2010/01/04 08:41:02, baiming wrote:
> Shouldn't these fields be private?
prefer to be protected.

http://codereview.appspot.com/181121/diff/1/2#newcode947
gtfsscheduleviewer/files/transit_editor.js:947: this.dom = $(id);
On 2010/01/04 08:41:02, baiming wrote:
> s/dom/elem

Done.

http://codereview.appspot.com/181121/diff/1/2#newcode949
gtfsscheduleviewer/files/transit_editor.js:949: this.createMap(this.dom);
On 2010/01/04 08:41:02, baiming wrote:
> In case that createMap doesn't successfully create the map, that means
this.map
> == null, should we throw an error (or return) to prevent from running the
below
> code and the further function calling of the object?

Done.

http://codereview.appspot.com/181121/diff/1/2#newcode993
gtfsscheduleviewer/files/transit_editor.js:993:
fetchStopsInBounds(map.getBounds());
On 2010/01/04 08:41:02, baiming wrote:
> For now the global variable "map" is still used by other objects, but within
the
> scope of the MapTool class, I hope we can avoid using the global one, instead
> use this.map.

Done.

http://codereview.appspot.com/181121/diff/1/2#newcode1009
gtfsscheduleviewer/files/transit_editor.js:1009: iconSelected = makeStopIcon();
On 2010/01/04 08:41:02, baiming wrote:
> 1) Is it possible to let iconSelected be a member of MapTool?
> 2) I prefer selectedIcon to iconSelected. It's up to you to choose.

for 1), we cannot right now.
for 2), i suggest we keep it before we remove them later.

http://codereview.appspot.com/181121/diff/1/2#newcode1024
gtfsscheduleviewer/files/transit_editor.js:1024: GEvent.addListener(map,
"moveend", this.onMoveEnd);
On 2010/01/04 08:41:02, baiming wrote:
> 1) s/map/this.map
> 2) Use GEvent.bind to use the prototype function of "this" as event handler.
> See http://code.google.com/apis/maps/documentation/events.html#Event_Bindings

Done.

http://codereview.appspot.com/181121/diff/1/2#newcode1025
gtfsscheduleviewer/files/transit_editor.js:1025: GEvent.addListener(map,
"zoomend", this.onZoomEnd);
On 2010/01/04 08:41:02, baiming wrote:
> Ditto.

Done.

http://codereview.appspot.com/181121/diff/1/2#newcode1026
gtfsscheduleviewer/files/transit_editor.js:1026: GEvent.addDomListener(window,
'resize', this.sizeRouteList);
On 2010/01/04 08:41:02, baiming wrote:
> GEvent.bind

currently of no needs to bind them, we only add global functions to be member
functions.

http://codereview.appspot.com/181121/diff/1/2#newcode1031
gtfsscheduleviewer/files/transit_editor.js:1031: * resize the route list on
window size change
On 2010/01/04 08:41:02, baiming wrote:
> s/resize/Resize

Done.

Sign in to reply to this message.

baiming

http://codereview.appspot.com/181121/diff/1/2 File gtfsscheduleviewer/files/transit_editor.js (right): http://codereview.appspot.com/181121/diff/1/2#newcode946 gtfsscheduleviewer/files/transit_editor.js:946: this.id = id; Being protected means it can also ...

14 years, 3 months ago (2010-01-04 11:43:51 UTC) #4

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

http://codereview.appspot.com/181121/diff/1/2#newcode946
gtfsscheduleviewer/files/transit_editor.js:946: this.id = id;
Being protected means it can also be accessed by the instances of MapTool's
subclasses. Do you really want MapTool to be inheritable? 

On 2010/01/04 10:09:43, calidion wrote:
> On 2010/01/04 08:41:02, baiming wrote:
> > Shouldn't these fields be private?
> prefer to be protected.
> 
>

http://codereview.appspot.com/181121/diff/1/2#newcode1009
gtfsscheduleviewer/files/transit_editor.js:1009: iconSelected = makeStopIcon();
On 2010/01/04 10:09:43, calidion wrote:
> On 2010/01/04 08:41:02, baiming wrote:
> > 1) Is it possible to let iconSelected be a member of MapTool?
Can you tell me a little more why we cannot do that?

> > 2) I prefer selectedIcon to iconSelected. It's up to you to choose.
> 
> for 1), we cannot right now.
> for 2), i suggest we keep it before we remove them later.
Agree.
>

http://codereview.appspot.com/181121/diff/1/2#newcode1026
gtfsscheduleviewer/files/transit_editor.js:1026: GEvent.addDomListener(window,
'resize', this.sizeRouteList);
On 2010/01/04 10:09:43, calidion wrote:
> On 2010/01/04 08:41:02, baiming wrote:
> > GEvent.bind
> 
> currently of no needs to bind them, we only add global functions to be member
> functions.

Since you refactored these global functions to members, they can't be used
independently/globally. And especially we changed the "map" to "this.map" in
onMoveEnd().

http://codereview.appspot.com/181121/diff/13/1004#newcode953
gtfsscheduleviewer/files/transit_editor.js:953: 
It makes no sense to use the try .. catch, and throw the original uncertain
error. Here we only want to handle the very specific case "this.map == null",
not others.

Let me explain why I would like to add this checking here: In the createMap()
function, we clearly know there is possibility that the map won't be
initialized. And we also want the callers know that, instead of letting the
programmers be confused why there's weird "null pointer" errors. 

As we cannot predict other types of errors, which may be thrown from setBounds()
or init(), so we shoudn't catch them.

I propose two options:

1) Throw an error with the detailed description to tell the caller what fails.
if (this.map == null) {
  throw new Error('Map cannot be created, may because the browser is not
compatible with Google maps API.');
}

2) Alert the message in option #1 and return, but not throwing error.

For option #2, we'd better off to also add a "if (this.map == null) { return; }"
 checking in the front of each member function. Because those function calls
will make no sense and cause errors.

Call me if you have questions.

Sign in to reply to this message.

baiming

On Mon, Jan 4, 2010 at 7:43 PM, <baiming@google.com> wrote: > > http://codereview.appspot.com/181121/diff/1/2 > File ...

14 years, 3 months ago (2010-01-04 23:58:24 UTC) #5

On Mon, Jan 4, 2010 at 7:43 PM, <baiming@google.com> wrote:

>
> http://codereview.appspot.com/181121/diff/1/2
> File gtfsscheduleviewer/files/transit_editor.js (right):
>
> http://codereview.appspot.com/181121/diff/1/2#newcode946
> gtfsscheduleviewer/files/transit_editor.js:946: this.id = id;
> Being protected means it can also be accessed by the instances of
> MapTool's subclasses. Do you really want MapTool to be inheritable?
>
>
> On 2010/01/04 10:09:43, calidion wrote:
>
>> On 2010/01/04 08:41:02, baiming wrote:
>> > Shouldn't these fields be private?
>> prefer to be protected.
>>
>
>
>
> http://codereview.appspot.com/181121/diff/1/2#newcode1009
> gtfsscheduleviewer/files/transit_editor.js:1009: iconSelected =
> makeStopIcon();
> On 2010/01/04 10:09:43, calidion wrote:
>
>> On 2010/01/04 08:41:02, baiming wrote:
>> > 1) Is it possible to let iconSelected be a member of MapTool?
>>
> Can you tell me a little more why we cannot do that?
>
>
>  > 2) I prefer selectedIcon to iconSelected. It's up to you to choose.
>>
>
>  for 1), we cannot right now.
>> for 2), i suggest we keep it before we remove them later.
>>
> Agree.
>
>
>
> http://codereview.appspot.com/181121/diff/1/2#newcode1026
> gtfsscheduleviewer/files/transit_editor.js:1026:
> GEvent.addDomListener(window, 'resize', this.sizeRouteList);
> On 2010/01/04 10:09:43, calidion wrote:
>
>> On 2010/01/04 08:41:02, baiming wrote:
>> > GEvent.bind
>>
>
>  currently of no needs to bind them, we only add global functions to be
>>
> member
>
>> functions.
>>
>
> Since you refactored these global functions to members, they can't be
> used independently/globally. And especially we changed the "map" to
> "this.map" in onMoveEnd().
>
> http://codereview.appspot.com/181121/diff/13/1004#newcode953
> gtfsscheduleviewer/files/transit_editor.js:953:
> It makes no sense to use the try .. catch, and throw the original
> uncertain error. Here we only want to handle the very specific case
> "this.map == null", not others.
>
> Let me explain why I would like to add this checking here: In the
> createMap() function, we clearly know there is possibility that the map
> won't be initialized. And we also want the callers know that, instead of
> letting the programmers be confused why there's weird "null pointer"
> errors.
>
> As we cannot predict other types of errors, which may be thrown from
> setBounds() or init(), so we shoudn't catch them.
>
> I propose two options:
>
> 1) Throw an error with the detailed description to tell the caller what
> fails.
> if (this.map == null) {
>  throw new Error('Map cannot be created, may because the browser is not
> compatible with Google maps API.');
> }
>
> 2) Alert the message in option #1 and return, but not throwing error.
>
> For option #2, we'd better off to also add a "if (this.map == null) {
> return; }"  checking in the front of each member function. Because those
> function calls will make no sense and cause errors.
>
> Call me if you have questions.


A new proposal, since all that MapTool does is to handle the maps API
object, how about adding such precondition in the front of the constructor:
function MapTool(id) {
  if (!GBrowserIsCompatible()) {
    alert('xxx');  // to let users know what happened
    return;  // to prevent initializing the object
  }
}

>
>
> http://codereview.appspot.com/181121
>



-- 
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.

calidion

copying createMap codes into constructor doesn't make any difference from the previous code. the existence ...

14 years, 3 months ago (2010-01-05 02:59:03 UTC) #6

copying createMap codes into constructor doesn't make any difference from
the previous code.
the existence of the alert or not will not prevent the whole process.
after new MapTool, we still have processes to proceed.

and also I think there is of use to catch any exception here.

Firstly, if GMap2 can not be accessed or failed to be created,
GMap2 will generate exceptions. or the GBrowserIsCompatible is not defined.

Secondly, if the Browser is not compatible, total process is meaningless.

Thirdly, if the Browser is not compatible, it is not a exception, it is a
condition.

Lastly, what should we catch for or any meaning we need this catch, what is
the exception we anticipate?

for all the process will be executed smoothly with the GMap2 Object created
successful
or totally failed, I think there is no reason to catch any exception here.
for the create of the MapTool can not prevent the process of the whole
program.




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

>
>
> On Mon, Jan 4, 2010 at 7:43 PM, <baiming@google.com> wrote:
>
>>
>> http://codereview.appspot.com/181121/diff/1/2
>> File gtfsscheduleviewer/files/transit_editor.js (right):
>>
>> http://codereview.appspot.com/181121/diff/1/2#newcode946
>> gtfsscheduleviewer/files/transit_editor.js:946: this.id = id;
>> Being protected means it can also be accessed by the instances of
>> MapTool's subclasses. Do you really want MapTool to be inheritable?
>>
>>
>> On 2010/01/04 10:09:43, calidion wrote:
>>
>>> On 2010/01/04 08:41:02, baiming wrote:
>>> > Shouldn't these fields be private?
>>> prefer to be protected.
>>>
>>
>>
>>
>> http://codereview.appspot.com/181121/diff/1/2#newcode1009
>> gtfsscheduleviewer/files/transit_editor.js:1009: iconSelected =
>> makeStopIcon();
>> On 2010/01/04 10:09:43, calidion wrote:
>>
>>> On 2010/01/04 08:41:02, baiming wrote:
>>> > 1) Is it possible to let iconSelected be a member of MapTool?
>>>
>> Can you tell me a little more why we cannot do that?
>>
>>
>>  > 2) I prefer selectedIcon to iconSelected. It's up to you to choose.
>>>
>>
>>  for 1), we cannot right now.
>>> for 2), i suggest we keep it before we remove them later.
>>>
>> Agree.
>>
>>
>>
>> http://codereview.appspot.com/181121/diff/1/2#newcode1026
>> gtfsscheduleviewer/files/transit_editor.js:1026:
>> GEvent.addDomListener(window, 'resize', this.sizeRouteList);
>> On 2010/01/04 10:09:43, calidion wrote:
>>
>>> On 2010/01/04 08:41:02, baiming wrote:
>>> > GEvent.bind
>>>
>>
>>  currently of no needs to bind them, we only add global functions to be
>>>
>> member
>>
>>> functions.
>>>
>>
>> Since you refactored these global functions to members, they can't be
>> used independently/globally. And especially we changed the "map" to
>> "this.map" in onMoveEnd().
>>
>> http://codereview.appspot.com/181121/diff/13/1004#newcode953
>> gtfsscheduleviewer/files/transit_editor.js:953:
>> It makes no sense to use the try .. catch, and throw the original
>> uncertain error. Here we only want to handle the very specific case
>> "this.map == null", not others.
>>
>> Let me explain why I would like to add this checking here: In the
>> createMap() function, we clearly know there is possibility that the map
>> won't be initialized. And we also want the callers know that, instead of
>> letting the programmers be confused why there's weird "null pointer"
>> errors.
>>
>> As we cannot predict other types of errors, which may be thrown from
>> setBounds() or init(), so we shoudn't catch them.
>>
>> I propose two options:
>>
>> 1) Throw an error with the detailed description to tell the caller what
>> fails.
>> if (this.map == null) {
>>  throw new Error('Map cannot be created, may because the browser is not
>> compatible with Google maps API.');
>> }
>>
>> 2) Alert the message in option #1 and return, but not throwing error.
>>
>> For option #2, we'd better off to also add a "if (this.map == null) {
>> return; }"  checking in the front of each member function. Because those
>> function calls will make no sense and cause errors.
>>
>> Call me if you have questions.
>
>
> A new proposal, since all that MapTool does is to handle the maps API
> object, how about adding such precondition in the front of the constructor:
> function MapTool(id) {
>   if (!GBrowserIsCompatible()) {
>     alert('xxx');  // to let users know what happened
>     return;  // to prevent initializing the object
>   }
> }
>
>>
>>
>> http://codereview.appspot.com/181121
>>
>
>
>
> --
> 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.

baiming

As we talked over phone, moving GBrowserIsCompatible outward to the caller level makes more sense. ...

14 years, 3 months ago (2010-01-05 03:08:34 UTC) #7

As we talked over phone, moving GBrowserIsCompatible outward to the caller
level makes more sense.
Thanks for making the change, good job!

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

> copying createMap codes into constructor doesn't make any difference from
> the previous code.
> the existence of the alert or not will not prevent the whole process.
> after new MapTool, we still have processes to proceed.
>
> and also I think there is of use to catch any exception here.
>
> Firstly, if GMap2 can not be accessed or failed to be created,
> GMap2 will generate exceptions. or the GBrowserIsCompatible is not defined.
>
> Secondly, if the Browser is not compatible, total process is meaningless.
>
> Thirdly, if the Browser is not compatible, it is not a exception, it is a
> condition.
>
> Lastly, what should we catch for or any meaning we need this catch, what is
> the exception we anticipate?
>
> for all the process will be executed smoothly with the GMap2 Object created
> successful
> or totally failed, I think there is no reason to catch any exception here.
> for the create of the MapTool can not prevent the process of the whole
> program.
>
>
>
>
> 2010/1/5 Ming Bai 白明 <baiming@google.com>
>
>
>>
>> On Mon, Jan 4, 2010 at 7:43 PM, <baiming@google.com> wrote:
>>
>>>
>>> http://codereview.appspot.com/181121/diff/1/2
>>> File gtfsscheduleviewer/files/transit_editor.js (right):
>>>
>>> http://codereview.appspot.com/181121/diff/1/2#newcode946
>>> gtfsscheduleviewer/files/transit_editor.js:946: this.id = id;
>>> Being protected means it can also be accessed by the instances of
>>> MapTool's subclasses. Do you really want MapTool to be inheritable?
>>>
>>>
>>> On 2010/01/04 10:09:43, calidion wrote:
>>>
>>>> On 2010/01/04 08:41:02, baiming wrote:
>>>> > Shouldn't these fields be private?
>>>> prefer to be protected.
>>>>
>>>
>>>
>>>
>>> http://codereview.appspot.com/181121/diff/1/2#newcode1009
>>> gtfsscheduleviewer/files/transit_editor.js:1009: iconSelected =
>>> makeStopIcon();
>>> On 2010/01/04 10:09:43, calidion wrote:
>>>
>>>> On 2010/01/04 08:41:02, baiming wrote:
>>>> > 1) Is it possible to let iconSelected be a member of MapTool?
>>>>
>>> Can you tell me a little more why we cannot do that?
>>>
>>>
>>>  > 2) I prefer selectedIcon to iconSelected. It's up to you to choose.
>>>>
>>>
>>>  for 1), we cannot right now.
>>>> for 2), i suggest we keep it before we remove them later.
>>>>
>>> Agree.
>>>
>>>
>>>
>>> http://codereview.appspot.com/181121/diff/1/2#newcode1026
>>> gtfsscheduleviewer/files/transit_editor.js:1026:
>>> GEvent.addDomListener(window, 'resize', this.sizeRouteList);
>>> On 2010/01/04 10:09:43, calidion wrote:
>>>
>>>> On 2010/01/04 08:41:02, baiming wrote:
>>>> > GEvent.bind
>>>>
>>>
>>>  currently of no needs to bind them, we only add global functions to be
>>>>
>>> member
>>>
>>>> functions.
>>>>
>>>
>>> Since you refactored these global functions to members, they can't be
>>> used independently/globally. And especially we changed the "map" to
>>> "this.map" in onMoveEnd().
>>>
>>> http://codereview.appspot.com/181121/diff/13/1004#newcode953
>>> gtfsscheduleviewer/files/transit_editor.js:953:
>>> It makes no sense to use the try .. catch, and throw the original
>>> uncertain error. Here we only want to handle the very specific case
>>> "this.map == null", not others.
>>>
>>> Let me explain why I would like to add this checking here: In the
>>> createMap() function, we clearly know there is possibility that the map
>>> won't be initialized. And we also want the callers know that, instead of
>>> letting the programmers be confused why there's weird "null pointer"
>>> errors.
>>>
>>> As we cannot predict other types of errors, which may be thrown from
>>> setBounds() or init(), so we shoudn't catch them.
>>>
>>> I propose two options:
>>>
>>> 1) Throw an error with the detailed description to tell the caller what
>>> fails.
>>> if (this.map == null) {
>>>  throw new Error('Map cannot be created, may because the browser is not
>>> compatible with Google maps API.');
>>> }
>>>
>>> 2) Alert the message in option #1 and return, but not throwing error.
>>>
>>> For option #2, we'd better off to also add a "if (this.map == null) {
>>> return; }"  checking in the front of each member function. Because those
>>> function calls will make no sense and cause errors.
>>>
>>> Call me if you have questions.
>>
>>
>> A new proposal, since all that MapTool does is to handle the maps API
>> object, how about adding such precondition in the front of the constructor:
>> function MapTool(id) {
>>   if (!GBrowserIsCompatible()) {
>>     alert('xxx');  // to let users know what happened
>>     return;  // to prevent initializing the object
>>   }
>> }
>>
>>>
>>>
>>> http://codereview.appspot.com/181121
>>>
>>
>>
>>
>> --
>> Ming Bai 白明
>> Software Engineer
>> Google Inc.
>> Tel(Office): 86 (10) 6250-3361
>> Tel(Cell): +86-159-0153-4908
>> Email: baiming@google.com
>>
>
>


-- 
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.

baiming

It looks much better now. Thanks for the changes. Only one comment left. http://codereview.appspot.com/181121/diff/23/25 File ...

14 years, 3 months ago (2010-01-05 03:56:27 UTC) #9

calidion

14 years, 3 months ago (2010-01-05 04:20:44 UTC) #12

updated to rev 41

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

> LGTM
>
>
> http://codereview.appspot.com/181121
>

Sign in to reply to this message.

Issue 181121: added Map Tool and merged functions into it.

Patch Set 1 #

Patch Set 2 : change function to func to avoid error from chrome #

Patch Set 3 : updated on comments #

Patch Set 4 : update #

Patch Set 5 : move GBrowserIsCompatible into global. #

Patch Set 6 : updated #

Patch Set 7 : updated #

Patch Set 8 : updated #

Messages