6
\$\begingroup\$

This is perfectly readable code to me, but I'm afraid it may be too confusing to others. What do you think?

Note that I'm not interested in validation of the input here (validation will happen long before we get to this line in the code), so assume it is blank or a valid non-negative integer.

It's modified from this Stack Overflow question.

var numberToAdd = parseInt($('#number-to-add').val()) || 500; // if a number is not provided, use 500
\$\endgroup\$
1
  • \$\begingroup\$ If that's confusing to the people you work with, time to find new people to work with. It's perfectly idiomatic JS. \$\endgroup\$
    – Dagg
    May 30, 2014 at 17:16

6 Answers 6

4
\$\begingroup\$

Seems fine syntactically and perfectly idomatic, but always, always supply a radix argument to parseInt:

var numberToAdd = parseInt($('#number-to-add').val(), 10) || 500;

To quote MDN:

radix: An integer that represents the radix of the [...] string. Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior. Different implementations produce different results when a radix is not specified.
[...]
If the input string begins with "0", radix is eight (octal) or 10 (decimal). Exactly which radix is chosen is implementation-dependent. ECMAScript 5 specifies that 10 (decimal) is used, but not all browsers support this yet. For this reason always specify a radix when using parseInt.

(emphasis in the original)

\$\endgroup\$
3
\$\begingroup\$

There is a lot of "syntax" on that line, so something like this could be clearer:

var domNumber = parseInt($('#number-to-add').val());
var numberToAdd = domNumber || 500;
\$\endgroup\$
1
  • \$\begingroup\$ That's a good point, actually. It removes the possible confusion of the || into a line all by itself. Sorry I couldn't mark both of you correct, but I gave you a +1 at least. \$\endgroup\$ Jun 1, 2014 at 2:35
2
\$\begingroup\$

I'm a bit late, but I'd like to add two remarks:

One of the things that can be confusing in this line is that you get the value via jQuery and convert it at the same time. I'd at least separate the jQuery out:

var value = $('#number-to-add').val(); // TODO Need better variable name
var numberToAdd = parseInt(value, 10) || 500; 

Actually thinking about it, why are you reading from the DOM here in the first place? You said you already have validated the value at this point, so you must of already read the value from the field before. You shouldn't be reading it from the field again, but use the validated value.

My second point: Instead of using parseInt you could use the unitary plus operator instead. Anyone familiar enough with JavaScript to understand the use of || should also understand that:

var value = $('#number-to-add').val();
var numberToAdd = +value || 500; 
\$\endgroup\$
3
  • 1
    \$\begingroup\$ That's another way to split it up, and just as valid. The lines end up about equal length, which may help a human reading it. I'd probably call the "value" variable something like "numberToAddString", to emphasize what the variable is and the fact that it is, at the moment, still a string. The reason I used parseInt rather then parseFloat or uniary plus is because it forces us into an integer. I realize that the validation probably already did, but I see no reason to trust it that much, and I think it makes the code more maintainable to explicitly force it to an integer. \$\endgroup\$ Jun 2, 2014 at 1:17
  • \$\begingroup\$ @GuySchalnat IMHO it's not just another way to split it up. My way specifically distinguishes between two separate concerns: reading input and processing. The other suggestion splits "in the middle" of processing, so you have one line still doing two things (reading input and some processing) and the other line doing the rest of the processing. \$\endgroup\$
    – RoToRa
    Jun 2, 2014 at 11:40
  • \$\begingroup\$ Separation of concerns is a good point, and a good general rule. Obviously, I didn't quite get it when I read your answer (although, looking back, I thought you explained it fairly well, just not as a general rule). I'm reading it from the DOM again because the validation code is in a separate, modular, shared JavaScript file that I don't want to tie a dependency back to this code or expose extra variables out of for a very small gain in execution time. \$\endgroup\$ Jun 2, 2014 at 15:20
0
\$\begingroup\$

This seems a pretty standard syntax pattern in scripting languages. I am not an expert in JS but I can even read it without any issues — Take the ‘number-to-add’ (not sure what that is but I assuem some DOM object) value and then parse it to an Int, if that fails, then take the 500 as a default. Am I right?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Close, but also, if the parse succeeds and is zero, then it also takes 500 as the default. \$\endgroup\$ May 30, 2014 at 17:23
0
\$\begingroup\$

Benefit of using +value or Number(value) is that return value for '5foo' will be NaN as it should while parseInt(value, 10) will return 5

\$\endgroup\$
1
  • \$\begingroup\$ That is true, but in this case, I have validators to take care of '5foo' and the like. In this case, I still like parseInt, although in other cases, I would be happy to use something like what you mentioned. There are other good references on the web that I found comparing parseInt, parseFloat, +, Number, and ~~. \$\endgroup\$ Jun 2, 2014 at 15:10
0
\$\begingroup\$

I cannot believe that no one here mentioned that jQuery is redundant and slow in this case.

//If no valid number, 500.
var numberToAdd = parseInt(document.getElementById("number-to-add").value, 10) || 500; 
\$\endgroup\$
1
  • 1
    \$\begingroup\$ My usual method of operation is to write code as consistent as possible, and only when necessary, optimize it for speed. While every library adds a bit of slowdown, I prefer using a library rather then native dom methods, and very much prefer being consistent in that use. If, for some reason, I found that using native dom methods improved the user experience (by, say, a tenth of a second or more), then I would optimize and comment in the code why I was optimizing. For that matter, I'd also optimize parseInt (which may not be the fastest) as well. \$\endgroup\$ Jun 2, 2014 at 15:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.