5
\$\begingroup\$

I have an array that contains n arrays and each of these arrays contain a different number of string elements.
Each string contains a key word like evar#, event# or prop# (where # is a number).

This is my goal:

  • return all the key words from a string that is different with at least one of the other arrays
  • return the key words that does not exist in at least one of the other arrays.

I implemented a solution but it needs some optimization ... I'm open to any suggestions

Note: I'm using Google Apps Script which only accept ES5 (the "classic style").


This is my code:

var allActions = [
    [
        "overwrite value of evar1 with page_url_query 'int_cmp'", 
        "set event1 to custom value '1'", 
        "set event2 to custom value '1'", 
        "overwrite value of evar2 with page_url", 
        "overwrite value of evar3 with contextdata.user_id", 
        "set event4 to eventid", 
        "set event3 to eventid"

    ], 
    [
        "overwrite value of prop3 with contextdata.phase", 
        "overwrite value of prop2 with contextdata.room", 
        "set event1 to custom value '1'", 
        "set event2 to eventid", 
        "overwrite value of evar5 with contextdata.queue", 
        "set event4 to eventid", 
        "overwrite value of evar6 with contextdata.audience", 
        "set event3 to eventid",
        "set event5 to custom value '1'"
    ], 
    [
        "overwrite value of evar4 with contextdata.no_challenges", 
        "overwrite value of prop3 with contextdata.user_type", 
        "overwrite value of evar7 with contextdata.interaction", 
        "set event2 to custom value '1'", 
        "set event3 to eventid",
        "set event4 to eventid", 
        "set event1 to custom value '1'", 
        "set event5 to custom value '1'"
    ]
]

    
var keyWords = ["campaign","evar","event","prop", "mvvar1", "mvvar2", "mvvar3",
              "purchase", "scOpen", "scView", "scAdd"];

var arrLen = [];
var different = [];
for(var i = 0; i < allActions.length; i++) {
  arrLen.push(allActions[i].length);
} 

var max = Math.max.apply(null, arrLen)
var maxValue = arrLen.indexOf(max);

for(elem in allActions[maxValue]) {
  for(var i = 0; i < allActions.length; i++) {
    if(i !== maxValue) {
      for(var j in allActions[i]) {
        var mainElem = allActions[maxValue][elem];
        var checkElem = allActions[i][j];
        if(mainElem !== checkElem) {
          for(var k = 0; k < keyWords.length; ++k) {
            if( (index = mainElem.indexOf(keyWords[k])) !== -1) {
              splittedStr = mainElem.substring(index, mainElem.length).split(' ', 1);
              if(splittedStr[0].indexOf("campaign") !== -1) {
                if(different.indexOf(splittedStr[0]) == -1) {
                  different.push(splittedStr[0]);
                }
              }
              if(splittedStr[0].indexOf("evar") !== -1) {
                if(different.indexOf(splittedStr[0]) == -1) {
                  different.push(splittedStr[0]);
                }
              }
              if(splittedStr[0].indexOf("prop") !== -1) {
                if(different.indexOf(splittedStr[0]) == -1) {
                  different.push(splittedStr[0]);
                }
              }
              if(splittedStr[0].indexOf("event") !== -1) {
                if(different.indexOf(splittedStr[0]) == -1) {
                  different.push(splittedStr[0]);
                }
              }
            }

            else if( (index = checkElem.indexOf(keyWords[k])) !== -1) {
              splittedStr = checkElem.substring(index, checkElem.length).split(' ', 1);
              if(splittedStr[0].indexOf("campaign") !== -1) {
                if(different.indexOf(splittedStr[0]) == -1) {
                  different.push(splittedStr[0]);
                }
              }
              if(splittedStr[0].indexOf("evar") !== -1) {
                if(different.indexOf(splittedStr[0]) == -1) {
                  different.push(splittedStr[0]);
                }
              }
              if(splittedStr[0].indexOf("prop") !== -1) {
                if(different.indexOf(splittedStr[0]) == -1) {
                  different.push(splittedStr[0]);
                }
              }
              if(splittedStr[0].indexOf("event") !== -1) {
                if(different.indexOf(splittedStr[0]) == -1) {
                  different.push(splittedStr[0]);
                }
              }
            }
          }
        }
      }
    }
  }
}
console.log(different);

Can I somehow join these two conditions into a single condition? The first if statement checks the keyWord from the array with most values, and the second if statement checks the keyWord from the other arrays.

        if( (index = mainElem.indexOf(keyWords[k])) !== -1) {
          splittedStr = mainElem.substring(index, mainElem.length).split(' ', 1);
          if(splittedStr[0].indexOf("campaign") !== -1) {
            if(different.indexOf(splittedStr[0]) == -1) {
              different.push(splittedStr[0]);
            }
          }
          ..................
        }

        else if( (index = checkElem.indexOf(keyWords[k])) !== -1) {
          splittedStr = checkElem.substring(index, checkElem.length).split(' ', 1);
          if(splittedStr[0].indexOf("campaign") !== -1) {
            if(different.indexOf(splittedStr[0]) == -1) {
              different.push(splittedStr[0]);
            }
          }
          ...................
        }
\$\endgroup\$
  • 1
    \$\begingroup\$ If I only understood what it does, I would rewrite it from scratch. For now I have modified it a little, primarily I replaced the entire forth inner loop with two calls to way simpler and shorter 6-line function. Here it is (ES6). \$\endgroup\$ – Przemek Apr 17 '17 at 20:22
  • \$\begingroup\$ Well, what a fun code to review, I think I finished my review for now, I hope it still brings you something a year after you posted your question :) \$\endgroup\$ – Icepickle Aug 16 '17 at 21:40
6
\$\begingroup\$

You have an array called keywords, which you seem to use, however, it doesn't match the current given specification, nl:

var keyWords = ["campaign","evar","event","prop", "mvvar1", "mvvar2", "mvvar3", "purchase", "scOpen", "scView", "scAdd"];

According to your specification, a keyword is defined by a name + a number. It can ofcourse be that eg: mvvar1 would be followed by a number, if that would be the case, I think you could add a comment to the array why these appear to be different as specified.

Variable naming

I think, in general your code is quite confusing to read. You seem to be using rather unusual names for your variables, some examples:

  • arrLen seems to contain an array, that in turn contain the length of the individual arrays in your allActions array
  • maxValue is the value of the longest array inside allActions
  • different contains an array of the items that you will presumably give out, however, it isn't a very meaningfull name

Commenting your source code

The variable naming make your code hard to read, one constantly have to jump back an think, what was that variable used for again. I hope you are still active here, and that when you re-read this code, you also feel like, wow, I should have used way more comments.

Of course you should not comment every line, but at some point you could give a reason why you first want to know what is largest array, before your real code begins.

I sometimes explain above a for loop in a one-liner what my code should do and if I have exceptions where I ignore certain values, I like to mention it if it isn't clear from the code, but then from an observer perspective.

Some observations

Finding the longest array element in your allActions array could be handled lots easier. I rather have the feeling that either you wanted to test playing around with the apply or Math.max

var arrLen = [];
var different = [];
for(var i = 0; i < allActions.length; i++) {
  arrLen.push(allActions[i].length);
} 

var max = Math.max.apply(null, arrLen)
var maxValue = arrLen.indexOf(max);

This you could extract to an own function if you like (I don't think I would need it when I check if I know understood your code correctly at the end of the response)

/**
 * @param ([[]]) targetArr contains an array of arrays
 * @returns The index with the longest sub array or -1 if no valid subArray was found
 */
function getLongestArray( targetArr ) {
  if (!targetArr || !Array.isArray( targetArr ) ) {
    // incorrect argument should throw an error
    throw 'ArgumentException: targetArr should be an array';
  }
  var i, maxLength = -1, indexOfLongest = -1;
  for (i = 0; i < targetArr.length; i++) {
    if  (!targetArr[i] || !Array.isArray( targetArr[i] )) {
      // sub element is not an array, continue
      continue;
    }
    if (maxLength <= targetArr[i].length) {
      maxLength = targetArr[i].length;
      indexOfLongest = i;
    }
  }
  return indexOfLongest;
}

True, the code is longer, however, it's commented, there is a clear definition of what the function will do, and what the user of the function can expect. Splitting up single parts of your code into there own functions is a good way of structuring your code, and promote re-usability.

Now, another instant observation I might have is your interesting mix of different for statements. Especially seen that they are so close together, I really wonder if there was a reason for you to mix it up, or again, you felt like experimenting different forms of iterating an array

// here for...in is used
for(elem in allActions[maxValue]) {
  // here normal for loop
  for(var i = 0; i < allActions.length; i++) {
    if(i !== maxValue) {
      // for...in again
      for(var j in allActions[i]) {
        var mainElem = allActions[maxValue][elem]; // <- property which could have iterated from 0 to length of allActions[maxValue]
        var checkElem = allActions[i][j]; // <- property which could have iterated from 0 to length of allActions[i]
        if(mainElem !== checkElem) {
          // normal for loop
          for(var k = 0; k < keyWords.length; ++k) {

Here you really have to be consistent with your choice. There is no reason why a for..in statement brings benefit to your code, as you are iterating arrays and not an object. It makes the code less readable if such unexpected twists are added.

Subtopic variable scoping

The use of the var keyword in JavaScript and it's scope has been confusing since the dawn of JavaScript. It is important to know that in JavaScript, var defined variables are not block scoped, but they are function scoped variables, as this small snippet demonstrates

function accessVariableOutsideOfForBlock() {
  for (var i = 0; i < 5; i++) {
  }
  // var is block scoped, i = 5
  console.log(i);
}

accessVariableOutsideOfForBlock();
// i is undefined, as it was defined inside the function
// so expect an error or undefined here (depending on strict mode)
console.log(i);

Why do I bring this up, well, some argue that one should only see 1 var statement per function inside javascript, which will contain all the variables that will be used through the function itself. For your code, it also makes some sense, as you define i several times, nl here:

for(var i = 0; i < allActions.length; i++) {

and here

// here for...in is used
for(elem in allActions[maxValue]) {
  // here normal for loop
  for(var i = 0; i < allActions.length; i++) {

You also define some variables only inside the for loops and although in most languages that would be fine, in JavaScript 5 they don't belong inside the for loops. ES6 really made huge improvements by adding the block scoped keywords let and const, so if you would have the chance to use them now, I would really suggest you do so.

So as a rule of thumb, keep all var definitions on the top of your function, or where you define the first variable.

Extract similar code to functions

Code duplication is one of the most annoying things in a code base that you could have. Even if you know your code perfectly, still changes to certain parts of your algorithm could be forgotten if you copied the code to other parts.

One piece of code that could definitely be extracted into a function, would be this beautiful block of duplication

if( (index = mainElem.indexOf(keyWords[k])) !== -1) {
  splittedStr = mainElem.substring(index, mainElem.length).split(' ', 1);
  if(splittedStr[0].indexOf("campaign") !== -1) {
    if(different.indexOf(splittedStr[0]) == -1) {
      different.push(splittedStr[0]);
    }
  }
  if(splittedStr[0].indexOf("evar") !== -1) {
    if(different.indexOf(splittedStr[0]) == -1) {
      different.push(splittedStr[0]);
    }
  }
  if(splittedStr[0].indexOf("prop") !== -1) {
    if(different.indexOf(splittedStr[0]) == -1) {
      different.push(splittedStr[0]);
    }
  }
  if(splittedStr[0].indexOf("event") !== -1) {
    if(different.indexOf(splittedStr[0]) == -1) {
      different.push(splittedStr[0]);
    }
  }
}

else if( (index = checkElem.indexOf(keyWords[k])) !== -1) {
  splittedStr = checkElem.substring(index, checkElem.length).split(' ', 1);
  if(splittedStr[0].indexOf("campaign") !== -1) {
    if(different.indexOf(splittedStr[0]) == -1) {
      different.push(splittedStr[0]);
    }
  }
  if(splittedStr[0].indexOf("evar") !== -1) {
    if(different.indexOf(splittedStr[0]) == -1) {
      different.push(splittedStr[0]);
    }
  }
  if(splittedStr[0].indexOf("prop") !== -1) {
    if(different.indexOf(splittedStr[0]) == -1) {
      different.push(splittedStr[0]);
    }
  }
  if(splittedStr[0].indexOf("event") !== -1) {
    if(different.indexOf(splittedStr[0]) == -1) {
      different.push(splittedStr[0]);
    }
  }
}

This is really one of the best examples that should be refactored to a method, and that could even be split into several methods. You have to ask yourself, what is variable, and what is fixed in my piece of code.

Well, if you look at it closely, both parts of the if condition seem to check if a keyword exist in a certain array, and in case it does, it will take the found element of the array, and split the words, and then it will check if these words are specific words, and if they are inside another array already, if not they will be added.

As a side note, this part of the code I have the most questions about, simply because I didn't actually step through the program yet, and I cannot seem to match the code with your specifications, yet, so I won't change the functionality here, I will just show how it could be extracted to one or more functions

Lets take a single statement to begin with, like this one:

 if(splittedStr[0].indexOf("campaign") !== -1) {
    if(different.indexOf(splittedStr[0]) == -1) {
      different.push(splittedStr[0]);
    }
 }

this seems to be repeated 8 times, and could be extract like so

function addValueWhenContainingWord(value, word, targetArr) {
  if ( value.indexOf( word ) !== -1 && targetArr.indexOf( value ) !== -1 ) {
     targetArr.push( value );
  }
}

This would already reduce the statements inside the if block to:

addValueWhencontainingWord( splittedStr[0], "campaign", different );
addValueWhencontainingWord( splittedStr[0], "evar", different );
addValueWhencontainingWord( splittedStr[0], "prop", different );
addValueWhencontainingWord( splittedStr[0], "event", different );

for both if blocks. But that should not be the end, you might as well extract the code that checks if the keyword exists in the array, and the extraction of the keyword.

In the end, the statement that you wrote can be reduced to this (now I think I understood your code while refactoring it)

if( (index = mainElem.indexOf(keyWords[k])) !== -1) {
  splittedStr = mainElem.substring(index, mainElem.length).split(' ', 1);
  if (different.indexOf(splittedStr[0]) == -1) {
    different.push(splittedStr[0]);
  }
} else if( (index = checkElem.indexOf(keyWords[k])) !== -1) {
  splittedStr = checkElem.substring(index, mainElem.length).split(' ', 1);
  if (different.indexOf(splittedStr[0]) == -1) {
    different.push(splittedStr[0]);
  }
}

The reason for this reduction is because of the following (I am assuming that your values inside the keyWords array are the once that should be outputted)

  • splittedString will contain an array of exactly 1 element
  • that element will contain the keyword (but not necessarily start with it)
  • you want to document all the keywords that are not in the different array yet

If my above reasoning is true, you shouldn't really care that it is a specific word, and only the keyWords[k] is the one that could be found. If you however only want to filter out those specific 4 keywords, well, you could either make sure your keyWords array only contains those 4, or you could check if keyWords[k] is one of the four you are checking before checking if it's part of any of the sub arrays

Observations on the code from an observer view

I cannot really say why you need 4 for loops to iterate the allActions array, and even if I could, I don't know why you wish to start from the largest subArray. This would definitely need some commenting from your part.

The loops are a real killer for performance, I don't know how many arrays in array your code will have in the production environment, but I can see this become problematic with larger arrays-in-array.

The code, as mentioned before, is very hard to read, and it seems to contain much experimental code, from reading it, I am even unsure if it does what you specified. For example, from your spec, I wouldn't expect event1 to be in the output, as it occurs in all subarrays

Your code can definitely be refactored into several smaller functions, and with appropriate changes, it is also possible to reduce the nr of loops it uses.

If you would have future requirement changes that adds keywords, you could get unexpected results with keywords like eventloop or startcampaign. I feel like the checks you are making to ensure it's a keyword are insufficient. Even more interestingly is that you never seem to check if your keyword is actually followed by a number as you specified. I am not a huge fan of regexes, but this seems to be a very good place for you to use them ;)

After re-reading your specification, I think it should simply say that it should return all keywords that don't occur in all other arrays.

Suggested rewrite

In the end the rewrite took a bit longer, cause I was still fighting with the specifications, nl the first one :) However, here is a rewrite which tries to offer some explanation.

I moved the variable definitions to the bottom of the script, as you can clearly see the code at the beginning then.

I also added comments to the code, however not as excessively as I could

// creates a pattern to use with regexpressions from all the keywords
// the pattern essentially says:
// - \\b word boundary
// - all keywords joined by '|' ('or' separator), enclosed by ()
// - \\d{1,} must be followed by at least 1 digit,
function createKeywordPattern(keywords) {
  return '\\b(' + keywords.join('|') + ')\\d{1,}';
}

// gets all unique keywords with nr from a string based on a pattern
function extractUniqueKeywordsFromSentence(sentence, pattern) {
  // catch all keywords, that have a word boundary and end with at least 1 digit
  var reg = new RegExp(pattern, 'g'),
    result = {};
  while (match = reg.exec(sentence)) {
    // makes sure unique keywords come back, sentence might contain
    // 5 times the same keyword, not interested in that part
    result[match[0]] = true;
  }
  // if no matches were found, it will return an empty object
  return result;
}

// gets all unique keywords per array, based on a pattern
function extractUniqueKeywordsFromArrayOfSentences(array, pattern) {
  var result = {},
    temp, i, k;

  for (i = 0; i < array.length; i++) {
    // todo: investigate if Object.assign is ES5
    result = Object.assign(result, extractUniqueKeywordsFromSentence(array[i], pattern));
  }

  return result;
}

// gets all keywords from the full array, and then counts how many times they occur
// throughout the different arrays
// interestingly, this satisfies the output you have in your program
function extractGroupedKeywordsPerSubArray(array, keywords) {
  var pattern = createKeywordPattern(keywords),
    i, result = {},
    temp, word;
  for (i = 0; i < array.length; i++) {
    temp = extractUniqueKeywordsFromArrayOfSentences(array[i], pattern);
    for (word in temp) {
      if (result[word]) {
        // increase it's count
        result[word]++;
        continue;
      }
      // create a new key with a count of 1
      result[word] = 1;
    }
  }
  return result;
}

// gets all words that are not available in all arrays (those that have a count smaller
// than array.length)
function getAllNoneRepeatingKeywords(array, keywords) {
  var items = extractGroupedKeywordsPerSubArray(array, keywords),
    word,
    length = array.length,
    result = [];
  for (word in items) {
    if (items[word] === length) {
      continue;
    }
    result.push(word);
  }
  return result;
}

var allActions = [
    [
      "overwrite value of evar1 with page_url_query 'int_cmp'",
      "set event1 to custom value '1'",
      "set event2 to custom value '1'",
      "overwrite value of evar2 with page_url",
      "overwrite value of evar3 with contextdata.user_id",
      "set event4 to eventid",
      "set event3 to eventid"
    ],
    [
      "overwrite value of prop3 with contextdata.phase",
      "overwrite value of prop2 with contextdata.room",
      "set event1 to custom value '1'",
      "set event2 to eventid",
      "overwrite value of evar5 with contextdata.queue",
      "set event4 to eventid",
      "overwrite value of evar6 with contextdata.audience",
      "set event3 to eventid",
      "set event5 to custom value '1'"
    ],
    [
      "overwrite value of evar4 with contextdata.no_challenges",
      "overwrite value of prop3 with contextdata.user_type",
      "overwrite value of evar7 with contextdata.interaction",
      "set event2 to custom value '1'",
      "set event3 to eventid",
      "set event4 to eventid",
      "set event1 to custom value '1'",
      "set event5 to custom value '1'"
    ]
  ],
  keywords = ['evar', 'campaign', 'prop', 'event'];

// executes and logs the result
console.log( getAllNoneRepeatingKeywords(allActions, keywords) );

I hope it still brings you something (yes, I know, I posted 1 year after the original question or something), however, I felt it was a good practice for me as well

\$\endgroup\$
  • \$\begingroup\$ Your doc is lying for getLongestArray, you are clearly counting on possibly not getting a list of arrays since you do a ton of type checking. If you were guaranteed a list of arrays, then the function would be a oneline. return targetArr.map( e => e.length ).sort().pop(); \$\endgroup\$ – konijn Aug 15 '17 at 21:32
  • \$\begingroup\$ @konijn the question is asking for es5 implementation, which doesn't have either arrow functions or map. I agree that the typechecking might be unnessary, however I am showing how certain things can be implemented, do you feel I am over doing that function or the review? \$\endgroup\$ – Icepickle Aug 15 '17 at 21:38
  • \$\begingroup\$ You are doing great, I just think that the doc should state that your function is hardened and will handle pretty much anything. Point taken about ES5 btw. \$\endgroup\$ – konijn Aug 15 '17 at 21:41
  • \$\begingroup\$ @konijn My jsdoc is a bit rusty, should I document the parameter as ([[]]|any)? It doesn't really handle every case as it would throw an exception in case the argument is not an array. Or does jsdoc have a @throws statement? \$\endgroup\$ – Icepickle Aug 15 '17 at 21:47
  • \$\begingroup\$ That is handling as well in my book ;) \$\endgroup\$ – konijn Aug 15 '17 at 21:48
0
\$\begingroup\$

To improve your code you need apply more the Single responsibility.

You don't need to reduce theses two conditionals to one, you can create a function and call this function in both.

var handleString = function(splittedStr, resultArray) {
    if(splittedStr[0].indexOf("campaign") !== -1) {
      // push the campaing
    }
    if(splittedStr[0].indexOf("evar") !== -1) {
      // push the evar
    }
    // continue
}

In this way your function will have less repeated code but your intention writing it remains the same, so in the future you (and your co-workers) will understand it better.

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