Properly check for 'undefined' instead of using || throughout codebase

[mramato]

In many places we use || instead of checking for undefined. For example

this.myValue = value || 12.0;

This is bad because a value of 0 will result in myValue equalling 12. The correct code is

this.myValue = typeof value !== 'undefined' ? value : 12.0;

@shunter wrote a simple function to make this easy, so it would be simplified to

this.myValue = defaultValue(value, 12.0)

Someone needs to go through the entire code base and look into all || usage, replacing non-boolean specific usage with these checks.

[andr3nun3s]

Was this issue solved?

I've looked around a bit and haven't found this. I found some calls to the defaultValue() function so I guess this was solved.

[mramato]

There are still places where this is an issue, but they are hard to track down so we have been fixing them when we find them. If you feel like going through the code and looking for them and fixing them, that would be awesome. It's not just this.myValue = value || 12.0; but also if(myValue) where myValue is not a boolean, that needs to be fixed.

[shunter]

One thing to keep in mind, though, is that the defaultValue(a, b) function can introduce performance problems if b is not a constant value or primitive value, because both a and b must be evaluated in order to call the function. In cases where b is an allocated object, we need to stick with the typeof a !== 'undefined' ? a : b pattern. See this discussion for more details.

[pjcozzi]

@Andre-Nunes if you are interested in this, you could start with a few files, which we could review to make sure you are heading in the right direction before updating everything. This would be a welcomed contribution.

[andr3nun3s]

@pjcozzi , I am really interested in this, it's a good way to start dwelling into the code base and prepare myself for GSoC.

I would really appreciate if someone pointed me in the right direction. For example on Cesium-hint.js:

var win = CodeMirror.cesiumWindow || window;
    if (context) {
      // If this is a property, see if it belongs to some object we can
      // find in the current environment.
      var obj = context.pop(), base;
      if (obj.className == "variable")
        base = win[obj.string];
      else if (obj.className == "string")
        base = "";
      else if (obj.className == "atom")
        base = 1;
      else if (obj.className == "function") {
        if (win.jQuery != null && (obj.string == '$' || obj.string == 'jQuery') &&
            (typeof win.jQuery == 'function'))
          base = win.jQuery();
        else if (win._ != null && (obj.string == '_') && (typeof win._ == 'function'))
          base = win._();
      }

Is wina suitable candidate for fix?

A different example this time on CubeMapEllipsoidTessellator.js

attributeName = attributeName || 'position';

This is probably not the best way to review code so any advice will be welcome.

[mramato]

Yes. Both win, context and attributeName are all cases where we should be performing a proper check.

[andr3nun3s]

I'll start by fixing those then. Should I open a pull request as soon as I fix those or that's just 'overkill'?

I've asked further questions about branching and "commit etiquette" on the forum. It's probability better to continue this discussion there.

[emackey]

Hang on, Cesium-hint.js is a special case. It's a modified copy of a third-party file, ThirdParty\CodeMirror-2.24\lib\util\javascript-hint.js, with some Cesium customizations. Because it's a fork of that file, I want to keep the customizations to a minimum, so it's upgradable. Please don't "fix" anything in that file, thanks!

[mramato]

@emackey Kind of offtopic but if that's the case, shouldn't Cesium-hint.js be in a Sandcastle/ThirdParty directory? And why not just keep it named javascript-hint.js?

[emackey]

Might be good to move it down into a new Sandcastle/Thirdparty folder, this issue seems to come up from time to time. Note that you'll have to mess with the jsHint options for Eclipse after you move or rename it. I do like the name Cesium-hint because the file contains more than just plain JavaScript hints, it's been customized for Cesium.

[mramato]

@Andre-Nunes For something like this you should either open a new issue or post a new message to the mailing list. Please do not post about an unrelated issue in some other issue, it makes things difficult to manage.

[andr3nun3s]

I believe this was solved with the above PRs, after #694 I've gone through most Source files as well, is there anywhere else I should look at?

[mramato]

While I'm sure there are some remaining cases that we'll find over time, I think the majority of them have been fixed. Thanks for the work @Andre-Nunes and @nobelium!