2
\$\begingroup\$

Some time ago I wrote a series of functions for turning a string of user information from multiple users 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)

My first problem was that 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 went overboard and made an entire function (selfToArray) to separate the user ID from the rest of the data.

var userinfoArray = new Array();    
var filterArray = new Array(); 
selfToArray( author_raw, filterArray ) //separate ID, keep the rest in the new array
if (filterArray.length > 0) {
    for (i = 0; i < (filterArray.length); i++) {
        author_data = filterArray[i].name
        getAuthor(author_data, userinfoArray, tile_AuthorArray_total)
    }
}

function selfToArray(input, inputArray, splitter) {
    if ( input ) {
        var input_id = ''
        var input_name = ''
        var splitby = ';'
        if (typeof splitter != 'undefined') splitby = splitter              
        if (input.indexOf( splitby ) >= 0) {
            var input_split_array = input.split( splitby )
            var input_split_array_length = input_split_array.length
            if (input_split_array_length > 1) {
                var second_value = input_split_array[1]
                var second_value_first_char = second_value.charAt(0)
                var second_value_second_char = second_value.charAt(1)
                if (second_value_first_char == '#') {
                    var number_check = isNaN(second_value_second_char) //it is a hash, and second char is not a number
                    for (var i = 0; i < input_split_array_length; i++) {
                        var this_id = ''
                        var name = ''
                        if (i % 2 == 0) { //check if even
                            this_id = input_split_array[i]
                            this_id = this_id.replace('#', '')
                            input_id = this_id
                        } 
                        else { //odd
                            name = input_split_array[i]
                            name = name.replace('#', '')
                            input_name = name
                            inputArray.push({
                                id: input_id,
                                name: input_name
                            });

                        }
                    }
                }
            }
        }
    }
}   

function getAuthor(input, inputArray, totalArray ) { 
    var this_name = ''
    var this_login = ''
    var this_email = ''
    var this_username = ''
    var this_SIP = ''
    var this_login = '' 
    var this_location = ''  
    if (input.indexOf( ',,' ) >= 0) {
        input = input.replace(/,,/g, 'dummyvalue');
    }
    var arr = input.split(',#')
    for(var i=0; i < arr.length; i++) {
        if (arr[i].indexOf("dummyvalue") >= 0) {    
            arr[i] = arr[i].replace('dummyvalue',',')                       
            if (arr[i].indexOf(" (") >= 0) {
                this_name = arr[i].split(' (')[0]
                this_location = arr[i].split(' (')[1] 
            }
            else {
                this_name = arr[i][0]                                   
            }
        }
        else if (arr[i].indexOf("@") >= 0) {
            this_email = arr[i]
            this_SIP = arr[i]
            this_username = arr[i].split('@')[0].toLowerCase()
        }
        else if (arr[i].indexOf("adfs") >= 0) {
            this_login = arr[i]
        }
    }
    inputArray.push({
        name: this_name,
        login: this_login,
        email: this_email,
        username: this_username,
        SIP: this_SIP,
        location: this_location
    });
    totalArray.push( this_name );               
}

I've learned enough to know that this is very, very far from an elegant way of doing this, but I am not certain of the optimal approach. I assume that first selfToArray() function should be easy to replace with either a regex filter or a simple loop, depending on what runs fastest.

\$\endgroup\$
2
\$\begingroup\$

From a once over:

  • Read this about the arrow pattern code, more specifically, your check on input should read like this:

    if( !input ) {
      return; //Or, you could even throw an error / exception ?
    }
    
  • This can be shortened with the OR operator:

    var splitby = ';'
    if (typeof splitter != 'undefined') splitby = splitter  
    

    to

    var splitby = splitter || ';'
    
  • input_split_array_length has the same length input_split_array.length and you use it only twice. Either give it a shorter name, or don't create that variable at all.
  • You could replace all these

    var this_name = ''
    var this_login = ''
    var this_email = ''
    var this_username = ''
    var this_SIP = ''
    var this_login = '' 
    var this_location = ''  
    

    with

    var author = {};
    

    and then simply use author.name instead of this_name. Similarly you can then replace your push statement with inputArray.push( author );

  • Semicolons, use them
  • var userinfoArray = new Array(); -> var userinfoArray = [] , which is more idiomatic
  • this_login is declared twice
  • You are declaring i in the global namespace here: for (i = 0; i < (filterArray.length); i++) { , use a tool like jshint.com to detect this and fix this
  • You determine number_check but you never use it
  • Naming
    • second_value_first_char -> secondValueFirstChar <- Use lowerCamelCase
    • As names go, secondValueFirstChar is kind of terrible, think about shorter more meaningfull names
  • Commenting, you dont have a lot of it, one comment is hard to understand and they all deserve their own line I think

Overall, I think you should take the above into account and rewrite large portions. Then, I would resubmit your re-factored code, because I think I only scratched the surface.

\$\endgroup\$
  • 1
    \$\begingroup\$ This. Also worth mentioning is line 4; if (filterArray.length > 0) { can be rewritten as if (filterArray.length) {. \$\endgroup\$ – Ben Sep 22 '14 at 22:48

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.