6
\$\begingroup\$

This is a revised and expanded version of Transforming complex strings into simple arrays

My function grabPerson turns strings of user information from multiple users to "business cards" on my webpage, via an array containing the information needed: Name, username and location.

The raw userinfo string (userinfo_raw) looks like this:

1074;#Fring,, Gustavo (US - New Mexico),#i:0ǵ.t|adfs|gfring,#gfring@LPH.com,#gfring@lph.com,#Fring,, Gustavo (US - New Mexico);#11903;#Rodarte-Quaylet,, Lydia (US - Houston),#i:0#.w|atrema\lrquaylet,#lrquaylet@madrigal.com,#,#,, Rodarte-Quaylet,, Lydia (US - Houston)

The users are separated with "#;", but this separator is also used within each user, separating the user ID from the rest of the information. I do not need the user ID, so I am using the function (selfToArray) to separate the user ID from the rest of the data.

function selfToArray(input, inputArray, splitter) { 
    var splitby = splitter || ';'               
    if (input.indexOf( splitby ) === -1 ) {
    }
    else {
        var input_split_array = input.split( splitby )
        if (input_split_array.length < 1 ) {
        }
        else if ( input_split_array[1].charAt(0) == '#') {
            for (var i = 0; i < input_split_array.length ; i++) {
                if (i % 2 !== 0) { //check if odd (even is the ID, no need for this now.)
                    inputArray.push( input_split_array[i].replace('#', '') )
                }
            }
        }
    }
}       

function getAuthor(input, inputArray ) { 
    var author = {};
    if (input.indexOf( ',,' ) >= 0) input = input.replace(/,,/g, 'dummyvalue'); //used for identifying the name later on
    var arr = input.split(',#')
    for(var i=0; i < arr.length; i++) {
        if (arr[i].indexOf("dummyvalue") >= 0) { //find section containing the name
            arr[i] = arr[i].replace('dummyvalue',',')                       
            if (arr[i].indexOf(" (") >= 0) { //if author name contains location, such as: Fring,, Gustavo (US - New Mexico)
                author.name = arr[i].split(' (')[0]
                author.location = arr[i].split(' (')[1]
            }
            else {
                author.name = arr[i][0]                                 
            }
        }
        else if (arr[i].indexOf("@") >= 0) { //find section with email
            author.email = arr[i]
            author.sip = arr[i]
            author.username = arr[i].split('@')[0].toLowerCase()
        }
        else if (arr[i].indexOf("adfs") >= 0) {
            author.login = arr[i]
        }
    }
    inputArray.push( author );          
}   

function grabPerson( author_raw ) { 
    var result_container = '' 
    var filterArray = []; //for storing the data after the ID separation
    var userinfoArray = []; //for collecting the results of processing this data
    selfToArray( author_raw, filterArray ) //separate ID, keep the rest in the new array
    if (filterArray.length) {
        for (var i = 0; i < (filterArray.length); i++) {
            getAuthor( filterArray[i], userinfoArray ) //transform userinfo into array
        }
    }
    if (userinfoArray.length) {
        result_container += '<ul class="persons_container">'
        for (var i = 0; i < (userinfoArray.length); i++) {
            var profilesizing = 0 //could utilize 'i' for gradual size increase/decrease
            var author_location = userinfoArray[i].location || ""
            if (author_location) author_location = " (" + author_location
            var author_name = userinfoArray[i].name + author_location
            if (author_name) {
                var result_person = userTile( userinfoArray[i].email, author_name, "inline", profilesizing, author_location)
                result_container += result_person   
            }
        }
        result_container += '<div style="display: block; clear: both;"></div>'
        result_container += '</ul>'
        return result_container //the results are collected and only later added to the DOM, in a single operation.
    }
}

Thanks to the feedback given to me by @konijn, I've improved the code somewhat, but I suspect that there are still optimizations that could be done in order to make this as fast as possible, since the hundreds (and potentially thousands) of rows are processed.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Is there a specific question you want help with (performance, code size, reduced complexity, architecture, robustness, bugs) that matter to you most? Or are you just asking for any generic code improvement suggestions? \$\endgroup\$
    – jfriend00
    Sep 23, 2014 at 8:01
  • 2
    \$\begingroup\$ I have to ask: Are those actual email addresses in your example? If so, you'll probably (read: definitely) want to censor those. Just saying. \$\endgroup\$
    – Flambino
    Sep 23, 2014 at 8:56
  • \$\begingroup\$ Performance is key here, @jfriend00 - hundreds and potentially thousands of iterations. \$\endgroup\$
    – Skarven
    Sep 23, 2014 at 10:00
  • 2
    \$\begingroup\$ I like what you did with the code, this starts to look like something I would be willing to maintain. You still suffer from arrow'itis \$\endgroup\$
    – konijn
    Sep 23, 2014 at 13:20
  • 1
    \$\begingroup\$ Great, I accidentally revealed to the world that I've yet to watch Breaking Bad... oh, the egg on my face :P \$\endgroup\$
    – Flambino
    Sep 23, 2014 at 14:52

1 Answer 1

3
\$\begingroup\$

General notes

  • Consistency, please. JavaScript is typically camelCase (and PascalCase for constructors). You do use that, but you also use underscored_names here and there. Please pick one style - preferably the JS standard one - and stick to it.

  • You're missing a lot of semicolons. I already mentioned it in a previous review, but I'd really just add those semis even if the code can run without them. Again, the automatic semicolon insertion is more of a crutch for lazy code than syntactic sugar.

About selfToArray()

  • The name is sort of strange. What's self? stringToArray would be more straight-forward although it's pretty generic and doesn't describe what kind of string it is. Still, better than selfToArray.

  • Your if..elsees are kinda funny, since you're not actually doing anything in the if blocks. Just flip the comparison around, and there's no need for an else block:

     if (input.indexOf( splitby ) !== -1 ) { <your code> }
    
  • Or, keep the current comparison, and just return right away. Again, no need for an else block

     if (input.indexOf( splitby ) === -1 ) { return; }
     <your code>
    
  • The typical way to set a default value for an argument is to reuse the argument's name, rather than make a new variable. It helps readability. So:

     splitter = splitter || ';';
     // or, alternative syntax
     splitter || (splitter = ';');
    

    By the way, I'd probably call it delimiter or separator instead (more conventional names).

  • Then again, I doubt you'll ever need to pass a custom delimiter. The rest of the code is tailored to some very specific data. It looks for # characters, and it discards every other part of the split string. It seems to me that you won't need a function that does all those very specific things - but with something other than a semicolon as the delimiter. (And indeed you don't use the function with a custom delimiter argument.)

  • Overall, I'm wondering why you need to pass an array to this function. It can just return one, all on its own. This isn't C where you need to pass pointers to pointers and stuff like that. To have any function modify its input is pretty iffy unless there's a really, really good reason. And I can't think of any since you can just handle the array it returns however you want (e.g. concat it to another array or something). String in, array out, period.

About getAuthor()

You're using some fragile magic here, it seems. You assume the name part will always contain ",," (I'd assume that that's there in case of a middle name), and that the login is always "adfs" (huh?). Oh, and that the word "dummyvalue" doesn't by chance appear anywhere else. But it might, who knows? Sure, it's very unlikely, but the code is still kludgy.

If there's any sort of logic to the data you're dealing with, the "rows" will always contain the same sort of information in the same order. In this case the order appears to be:

  1. Name and location
    1. Last name
    2. Middle name (presumably)
    3. First name
    4. Location in parantheses
  2. Login (I assume)
  3. Email
  4. Secondary email? Not sure, and either way it appears optional
  5. Name again, apparently.

If indeed that's the pattern (and I'll assume here that it is), you could just convert an input string to an array of "author objects" wholesale, and skip the selfToArray. A tiny internal function can handle the name/location parsing.

function getAuthors(input) { // note: plural, since it returns an array
  // internal function to parse the name/location
  function getNameAndLocation(string) {
    var parts = string.match(/([^,]*),\s*([^,]*),\s*(.*?)\s+\(([^\)]*)/);
    if(!parts) {
      return null; // oops, couldn't match name+location
    }
    return {
      name: {
        first: parts[3],
        middle: parts[2],
        last: parts[1]
      },
      location: parts[4]
    };
  }

  // this replaces selfToArray()
  var rows = input.split(/;#?/).filter(function (_, i) { return i % 2; });

  return rows.map(function (row) {
    var author = {},
        parts = row.split(/,#/),
        nameAndLocation = getNameAndLocation(parts[0]);

    author.name = nameAndLocation.name;
    author.location = nameAndLocation.location;
    author.login = parts[1];
    author.email = author.sip = parts[2]; // p.s. what is "sip"? Not clear.
    author.username = parts[2].split('@')[0];

    return author;
  });
}

For your example, it returns:

[
  {
    name: { first: 'Gustavo', middle: '', last: 'Fring' },
    location: 'US - New Mexico',
    login: 'i:0ǵ.t|adfs|gfring',
    sip: 'gfring@LPH.com',
    email: 'gfring@LPH.com',
    username: 'gfring'
  },
  {
    name: { first: 'Lydia', middle: '', last: 'Rodarte-Quaylet' },
    location: 'US - Houston',
    login: 'i:0#.w|atremalrquaylet',
    sip: 'lrquaylet@madrigal.com',
    email: 'lrquaylet@madrigal.com',
    username: 'lrquaylet'
  }
]

(p.s. there's a backslash in Lydia's login-info, which I think is supposed to be a pipe character |, right?)

As mentioned, the sip property isn't really explained, and since it's set to just be the email address, I don't see the need for it. It's just duplication.

But anyway, now you've got structured data to work with.

About grabPerson()

  • Strange name. It sounds like a whacky synonym for getAuthor. But it isn't. It generates HTML, so perhaps its name should reflect that?

  • Doing HTML strings is easy - but also error-prone. If the text you're concatenating contains characters like < or >, you're creating invalid HTML. It can get weird in a hurry (besides, all those strings aren't terribly readable). Better to create actual DOM objects, since that's what the DOM's for: An object-oriented interface for HTML. The elements don't need to be inserted anywhere; you can just create them in memory for later insertion.

  • You're inserting a div into a ul element, which is invalid HTML. I know its purpose is to clear floats, but at least use a special li for that. Or, better yet, use the CSS "clearfix" trick.

I won't do a full rewrite, but something like this should get you started:

function buildAuthorList(authors) {
  var list = document.createElement("ul");
  list.className = "persons_container";

  for(var i = 0, l = authors.length ; i < l ; i++) {
    var person = document.createElement("li");
    person.textContent = authors[i].name.first + " " + authors.name.last;
    list.appendChild(person);
  }

  return list; // return an in-memory DOM element, ready for insertion
}

However, I'd highly recommend using a library for cross-browser compatibility when it comes to (large-scale, repeated) DOM manipulation. These days, that usually means "just use jQuery". There are a lot of spiky pitfalls when it comes to the DOM and how different browsers implement it (textContent, for instance, didn't appear in IE until IE9). In other words: There's a reason jQuery is as huge as it is.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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