Separating values in string and returning them as comma separated string

Question

I am iterating through a set of values that are in the following string format:

69;#Statements;#44;#Clarifications;#45;#Suggestions;

(fields with multiple values returned from SharePoint.)

I've created a function that both adds these values to a list and returns them as a comma separated string:

Category = formatLookup( Category_Raw, categoryRow ) 

The function works, but is probably far from efficient. Any suggestions on how to optimize this function, used for thousands of elements?

function formatLookup( input ) {
    var result = ''
    if (typeof input != "undefined" && input != "" ) {
        if (input.indexOf(";#") >= 0) { //detect if there is more than one value in column      
            if (input.match(/;#/g).length > 1) {
                var array = input.split(';#');
                for (i = 0; i < array.length; i++) {
                    if (array[i].match(/^\d+/)) { //do nothing
                    } else {
                        var output = array[i].replace(/^\d+/, '')
                        if (output != '') {
                            result += output
                            result += ', '
                        }
                    }
                }
            } else {
                result = output = input.split(';#')[1]                              
            }
        } else {
            result = input
        }
        if (result.slice(-2) == ', ') result = result.substr(0, result.length - 2) //if the last item has a comma, remove this
    }
    return result
}

Answer 1

Stuff I noticed:

1. 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).

2. 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.

3. Strict comparisons (=== and !==). You might as well use them.

4. 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.

5. 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.

6. 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; }
   // ...
   

7. 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.

8. Don't glue together a string yourself; use join(', '). Saves you the trouble of going back and chopping off stray commas and whitespace afterward.

9. 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?

10. 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 789 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*;#/); // => [ '', 'Statements', '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:

input.replace(/\d+;#/g, ''); // => "Statements;#Clarifications;#Suggestions;"

so now you can split that string easily.

Your example input does however have 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.

Assuming 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:

input.match(/[a-z]\w+/ig) // matches a starting letter, and goes until it hits something that's not a letter or number.

which, if that's the appropriate pattern to use for your data, reduces the entire function to this:

function formatLookup(input) {
  return input.match(/[a-z]\w+/ig).join(', ');
}

Done.

Edit: Here's a more liberal regex (see comments)

function formatLookup(input) {
  return input.match(/([^#]*?)[a-z][^;]*/ig).join(', ');
}

This one will match everything between a "#" and a ";" provided there's just a single letter in there (i.e. it won't match all-number parts). It's still not perfect, and will for instance not match anything in an input string like "45;#3. 555-3443;" (don't know if that's relevant, but caveat emptor, etc.).

If that proves problematic, it might well be simpler to just split the string, and discard every 2nd part (if indeed the number-text-number-text... pattern holds true):

function formatLookup(input) {
  return input.split(/;#?/).filter(function (value, i) {
    return value && i % 2;
  }).join(', ');
}

Note that the above will likely be slightly faster (i.e. probably-not-worth-it faster) if you use an old-school for-loop to loop through and push to an array, rather than use filter:

function formatLookup(input) {
  var values = input.split(/;#?/),
      result = [],
      i = 1,
      l = values.length;

  for(; i < l ; i += 2) { // notice the increment
    if(values[i]) { result.push(values[i]); }
  }

  return result.join(', ');
}

Addendum: In an earlier iteration of the question, you were adding the values to a global array as well. If that's still useful, then I'd suggest simplifying the function even further, and just let it return an array:

function parseLookup(input) { // note the different name
  return input.match(/[a-z]\w+/ig);
}

Then, where needed, you can do something like this

var values = parseLookup(input);

someGlobalArray = someGlobalArray.concat(values); // append to global array

var formatted = values.join(", "); // same end-result as before

Even if you don't need the global array, I'd still suggest separating parsing the input, and formatting the output. Your current function does both, but it'd be cleaner and more modular to have one function for one thing, and another function for the other (or just inline the join(...) if it's only used once). Your current function basically takes all the data from one informal string format... and outputs another informal string format. Sure, something happens along the way, but overall it's a bit like two steps forward, one back.