Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly checked for 'undefined' bug #694

Merged
merged 17 commits into from
Apr 26, 2013
Merged

Conversation

nobelium
Copy link
Contributor

I have gone through almost all the files in core, renderer, scene and dynamicscene..

if (!variable) --> if(typeof variable === 'undefined')
var a = a || b; --> var a = defaultValue(a,b);

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 25, 2013

Thanks @nobelium. Have you sent in a CLA yet? See CONTRIBUTING.md.

@mramato any chance you want to review this? @Andre-Nunes has also done good work on this (#684) so this will need some coordination. In many places, we would benefit from both of their changes, but the merge will be some work.

@nobelium I took a quick glance at the code, and one thing I noticed is we generally do not use defaultValue if constructing the second parameter is expensive, e.g., it allocates something, unless the function is only called rarely (just ask in cases you are not sure). Instead, we use (typeof foo !== 'undefined') ? foo : new Foo().

@mramato
Copy link
Contributor

mramato commented Apr 25, 2013

@nobelium In addition to sending in the CLA, can you please merge in the head of Cesium master? @Andre-Nunes beat you to the punch for some of these and we need to reconcile them before it can be merged.

@nobelium
Copy link
Contributor Author

@pjcozzi Thanks for the tip. I have changed those lines.
@mramato I have merged the head of Cesium/master , and thanks for the jsHint tip. I used to check for errors using Jasmine.

Regarding CLA, I'm from India, its 23:45 here, I will be able to send it only after 8-10 hrs.

@nobelium
Copy link
Contributor Author

I have sent my CLA to cla@agi.com , Sorry for the delay, I thought I had to send a physically signed CLA.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 26, 2013

@nobelium we have your CLA now. Thanks.

@@ -313,7 +315,7 @@ define([
*/
CustomSensorVolume.prototype.update = function(context, frameState, commandList) {
this._mode = frameState.mode;
if (!this.show || this._mode !== SceneMode.SCENE3D) {
if (typeof this.show === 'undefined' || this._mode !== SceneMode.SCENE3D) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, the original code is actually what we want. this.show is actually a boolean variable. With the check you added, this code executes even when show is false. That's what make this cleanup a little trickier than normal, you need to make sure boolean values stay the same.

@shunter
Copy link
Contributor

shunter commented Apr 26, 2013

This looks good to me. Tests pass. One minor whitespace warning, which I'll fix in master. Thanks for your contribution, @nobelium! Merging.

@shunter
Copy link
Contributor

shunter commented Apr 26, 2013

I'll also fix @mramato's comment about the show, which he's correct about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants