HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Find differences and missing elements from multiple arrays

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
findarrayselementsdifferencesmultiplemissingandfrom

Problem

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

Solution

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



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 al

Code Snippets

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);
/**
 * @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;
}
// 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) {
for(var i = 0; i < allActions.length; i++) {

Context

StackExchange Code Review Q#141776, answer score: 6

Revisions (0)

No revisions yet.