10

Getting Unexpected "." from jslint ( http://jslint.com/ ) on this code:

function test(foo) {
    "use strict";
    return (foo || "").replace("bar", "baz");
}

Why does jslint have a problem with the || operator to force an empty string so a replace can be performed without causing an error, in case foo is passed in as undefined?

This passes:

function test(foo) {
    "use strict";
    var xFoo = (foo || "");
    return xFoo.replace("bar", "baz");
}

I know it's opinion based and I can ignore it, etc... but trying to understand why chaining like this is frowned upon. Also know about eshint, but I'm not trying to get around this message, just want to understand why.

Seems like the first approach is more concise and cleaner since it doesn't need the extra variable (xFoo).

Both functions do exactly the same thing under all conditions.

ciso
  • 2,887
  • 6
  • 33
  • 58
  • 3
    That looks like a bug in JSLint. – SLaks Jan 18 '16 at 01:26
  • [jsHint](http://jshint.com/) has no issue with your first function. – jfriend00 Jan 18 '16 at 01:28
  • It seems to be a bug. If it were opinion based, you would get another error clearly stating that it's frowned upon based on your lint configuration. – mostruash Jan 18 '16 at 01:29
  • 2
    idk specifically why jslint has a problem but it's not good code. if you pass `true` or `1` into it you will have a problem. – I wrestled a bear once. Jan 18 '16 at 01:31
  • @ChrisGciso - jslint isn't meant for testing (evaluating) code, but if there is something blatantly wrong it'll let you know. – I wrestled a bear once. Jan 18 '16 at 01:45
  • IMO the clearest option would be `foo = foo || ""; return foo.replace(...)` this seems more idiomatic – djechlin Jan 18 '16 at 02:12
  • Yes, this is one of those times where it's "good." – djechlin Jan 18 '16 at 03:02
  • 2
    @ChrisGciso - If you know that `foo` will either be falsey or a string, then there is absolutely nothing wrong with the code you have. It's not a syntax error. It's not even a programming error if you know something about the possible values of `foo` or if you want it to throw an exception if an invalid `foo` is passed. So, in many cases, using the code in the accepted answer is JUST a way to make the warning go away, not something that fixes anything that was actually wrong. – jfriend00 Jan 18 '16 at 03:13
  • 1
    @ChrisGciso - So, JSLint is forcing you to deal with something that might be perfectly fine code while JSHint has apparently decided that it doesn't think that is worth complaining about. Neither is really right or wrong - these are opinions. The only actual right and wrong comes when one knows how you want the function to behave if/when an invalid value is passed for `foo` or whether that can even happen. – jfriend00 Jan 18 '16 at 03:15
  • 1
    @ChrisGciso fwiw, _"JSLint takes a JavaScript source and scans it. If it finds a problem, it returns a message describing the problem and an approximate location within the source. The problem is not necessarily a syntax error, although it often is. JSLint looks at some style conventions as well as structural problems. It does not prove that your program is correct. It just provides another set of eyes to help spot problems. JSLint defines .., a stricter language than that defined by the ECMAScript.. JSLint will reject most legal programs."_ http://jslint.com/help.html – guest271314 Jan 18 '16 at 05:42
  • [Here is a great answer](http://stackoverflow.com/a/36586200/778975) to a similar/related question. – Useless Code Apr 13 '16 at 06:48

3 Answers3

1

It may be because it believes (foo || "") will evaluate to a boolean expression, so something like false.replace() would not make sense. Even though, yes, in your case you get a variable or empty string instead.

ScriptedPixels
  • 19
  • 4
  • it would never be `false` but it could be `true` – I wrestled a bear once. Jan 18 '16 at 01:32
  • I'm not saying it will evaluate as boolean, just that JSLint may believe that it will because it looks like a boolean expression. Also, if `foo` is `false`, `undefined`, etc. then the expression would use `""`, which is also falsey. How might it be `true`, if both cases could evaluate to `false`? – ScriptedPixels Jan 18 '16 at 01:52
1

Using String() contructor removes error at jslint

function test(foo) {
    "use strict";
    return String(foo || "").replace("bar", "baz");
}

See also Distinction between string primitives and String objects , JSLint Help

guest271314
  • 1
  • 15
  • 104
  • 177
  • 1
    @ChrisGciso This also passes `return foo && foo.replace("bar", "baz");` – guest271314 Jan 18 '16 at 02:28
  • @ChrisGciso Appear method attached to string literal notifies as warning at jslint ? `function test(foo) { "use strict"; if (!foo) { return "".replace("bar", "baz"); } }` , `function test(foo) { "use strict"; if (!foo) { return "bar".replace("bar", "baz"); } }` – guest271314 Jan 18 '16 at 02:37
  • 3
    @ChrisGciso - I wouldn't say this is the "proper" way to code. This is the way to make jsLint stop complaining. jsLint is not God here. I may write a function like that and WANT it to throw an exception if someone passes `true` to it because that's an invalid value that I don't want coerced into a string. In that case, this new method is wrong. – jfriend00 Jan 18 '16 at 03:16
  • 1
    This answer makes no sense. It talks about string objects and a `String` constructor, but does make use of neither in the given code. – Bergi Jan 18 '16 at 04:47
  • 1
    This is not the proper way to stop jsLint from complaing, it's just one of the ways to make it stop complaining. And this has nothing to do with the `String` function at all; it's all about the function call - `Number(foo || "").replace(…)` passes just as well. – Bergi Jan 18 '16 at 04:48
  • @ChrisGciso: I understand that using `String` is a nice and fitting workaround - it explicitly casts to the expected type which adds safety and annotates the code; and it also makes the warning go away. But it's wrong to state that jsLint complains about the ambiguity and that casting the value solves this problem - jsLint simply has a bug at parsing the syntax; and using a function call resolves that - it might as well be `couldReturnAnything(foo || "").replace(…)`. – Bergi Jan 18 '16 at 05:19
1

You could just make it two lines.

function test(foo) {
    "use strict";
    foo = foo || "";
    return foo.replace("bar", "baz");
}

There is no need to create a temporary xFoo variable. The foo parameter is a copy of the argument that was passed in since JavaScript does not support passing-by-reference.

It looks like what you are trying to do here is provide a default parameter. In that case I would make it crystal clear what you are doing by being even more explicit and type-checking it:

function test(foo) {
    "use strict";
    if (foo === undefined) {
      foo = "";
    }
    return foo.replace("bar", "baz");
}

Yes it is less succinct, but it will leave less room for the intention of the code to be misinterpreted by someone who reads it later. Explicitly checking the type also allows you to handle other potential problems.

function test(foo) {
    "use strict";
    if (foo === undefined) {
      foo = "";
    } else if (typeof foo !== 'string') {
      throw('foo must be a string');
    }
    return foo.replace("bar", "baz");
}

If you are using ES2015, you could also use a default parameter:

function test(foo = "") {
    "use strict";
    if (typeof foo !== 'string') {
      throw('foo must be a string');
    }
    return foo.replace("bar", "baz");
}

For pretty much any project I would suggest adding Babel to your build process so you can use default parameters and the many other useful features ES2015 adds to the language. Using Babel allows you to use them now without having to wait for all of the browsers to implement them.

Community
  • 1
  • 1
Useless Code
  • 12,123
  • 5
  • 35
  • 40