Missing semicolons. Perhaps the code doesn't need them, but you certainly want them. My argument is always that if JS truly didn't need semi-colons, they'd be completely optional (or cause outright syntax errors). But JS's automatic semi-colon insertion is (in my opinion) a crutch meant to fix bad code; ideally it should never come into play at all (unless you're a JS minifier or something).
You've got an implicit global: i
is never declared. And i
is veery likely to used in loops elsewhere, so you've got a really tricky bug that's just waiting to strike.
You've also got some other declaration-confusion: output
is declared in one branch of the code, but is also used in another branch. Since the branches are mutually exclusive, the latter branch is basically using output
without it having been declared. Again, JS does it's best to fix your code, but it shouldn't be necessary.
Just declare all your local variables at the top of the function.
Strict comparisons (===
and !==
). You might as well use them.
Also be consistent with your use of double- and single-quotes. There's no reason to mix and match. Doens't do any harm, but neither does consistency.
Your first conditional can be written as simply: if(input) { ...
. You're checking for undefined
and an empty string, but both of those are false'y, so it's needlessly complex. If anything, you could consider checking typeof input === 'string'
, if you want to check types.
But instead of wrapping everything in that if
block, just return early, and save yourself a lot of indentation.
var result = '';
if(!input) { return result; }
// ...
In a similar vein: You check if the input contains the ;#
delimiter. If it doesn't, result
is just set to be the input. So once again, just return early; send the input right back.
Don't glue together a string yourself; use join(', ')
. Saves you the trouble of going back and chopping off stray commas and whitespace afterward.
This doesn't make sense to me:
if (array[i].match(/^\d+/)) {
//do nothing
} else {
var output = array[i].replace(/^\d+/, '')
So, if array[i]
begins with some digits, do nothing. If it doesn't begin with digits, then... remove those non-existent digits? What?
Most of all: If you're going for performance, don't use indexOf
, match
, replace
and split
over and over to find the same stuff.
I'd say just split the input string right away. There's no reason to branch and check whether there's a reason to split; just split, and process the resulting array. If there's only one item in the array, so be it - no need to treat it any differently than if there were 2, 3, or 234789 items in the array.
It also looks to me like you intend to discard all the parts that are just digits (e.g. "69", "44" and "45" in your example), and just keep the text parts. If the pattern is always "number;#text;#number;#text ... etc", you could simply include the numbers in your split-pattern:. The code below will split the string at ;#<number>;#
, <number>;#
or just ;#
input.split(/\d+#;(?:;#)?\d*;#/); // => [ '', 'Statements;#''Statements', 'Clarifications;#''Clarifications', 'Suggestions;' ]
though, as you can see, that leaves you with a blank string in the array, because your input begins with a number.
Alternatively, you can replace all the numbers ahead of time:
Your example input does however hashave a trailing ;
which I don't see being handled anywhere. You current function returns a string with that semicolon intact ("Statements, Clarifications, Suggestions;"
), which is doubt is the intention.
Given thatAssuming you only want the "words", you could flip the whole thing around, and, instead of splitting, use match
to just pick out the parts you want, ignoring the numbers and delimiters: