Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This seems a bit redundant to me, but I'm not sure of how else I might be able to write this. Normally I'd use a switch statement, but I don't think that'll work here. I realize I could also do this in a for loop, but the main heart of the question is what's inside the loop itself. "arr" is an array, just in case it's not obvious.

var i = 0, x = arr.length;

while (i < x) {
    if (typeof arr[i] === 'object' && !Array.isArray(arr[i])) {
        attr = arr[i];
    }
    else if (typeof arr[i] === 'string') {
        text = arr[i];
    }
    else if (typeof arr[i] === 'object' && Array.isArray(arr[i])) {
        child = arr[i];
    }
    i++;
}
share|improve this question
2  
Array.isArray will also work with non-objects. You don't need to check if it's an object. –  Prinzhorn Mar 4 at 13:05
    
I'd remove the x variable altogether; since arr.length is short and clear enough by itself. –  Zar Mar 4 at 21:11
    
@Prinzhorn Ah yes, good point. Duly noted and update in my dev environment. Thank you! @Zar If I don't cache arr.length to l, doesn't it have to calculate arr.length on each iteration? –  DondeEstaMiCulo Mar 4 at 22:47
    
What should happen if typeof arr[i] is neither string nor object? (For instance, numbers, booleans, and undefined.) –  Zack Mar 5 at 15:33
    
@Zack The function that this loop is in serves a very specific purpose, and in my particular case there won't be any numbers or booleans. So if the values aren't a string or object they are just ignored, which is my intention. –  DondeEstaMiCulo Mar 5 at 21:26

9 Answers 9

I think at the very least I'd rearrange the cases to make the dichotomy between Array.isArray and !Array.isArray more direct and apparent:

while (i < x) {
    if (typeof arr[i] === 'string') {
        text = arr[i];
    }
    else if (typeof arr[i] === 'object') {
        if (Array.isArray(arr[i])) {
            child = arr[i];
        }
        else {
            attr = arr[i];
        }
    }
    i++;
}

That's not really a lot shorter but I think it makes the intent rather more clear (at least assuming I've divined the real intent correctly).

share|improve this answer

I would do it this way:

arr.forEach(function (item) {
    if (Array.isArray(item)) {
        attr = item;
    }
    else if (typeof item === 'object') {
        child = item;
    }
    else if (typeof item === 'string') {
        text = item;
    }
});

By using arr.forEach there is no need to subscript (arr[i]). You just get passed the item.

Array.isArray works just fine with anything you pass it, so there is no need to check before if the thing is or isn't an object.

By checking “arrayness” first, then you know for sure that if typeof item === 'object' it is not an array. It simplifies the logic.

share|improve this answer
    
Excellent suggestion. I actually had no idea that this even existed. However, since it's only compatible with IE9+, I'm afraid I can't use it. It needs to be compatible with older version as well (which I admittedly did not mention in my post). Upvoting anyway for its usefulness. :) –  DondeEstaMiCulo Mar 4 at 23:02
    
+1 good call on the Array.isArray –  megawac Mar 5 at 0:04

Along with @jerrycoffin and @RobertoBonvallet's Array.isArray suggestions, I would write this as a for loop 10 times out of 10 (this is the conventional for loop case afterall). I tend to use for loops whenever I'm iterating every item of a single collection and a while loop for any other purpose (but either can always be written as the other so w/e floats your boat).

I would also probably cache the result of typeof arr[i] and if I were using arr[i] anywhere else in the loop I would probably make a variable for it as well. I usually consider using a switch if I use 3 or more typeof item so if you extend further that may be something to consider

Also x is a cryptic name for a loop control variable. Use l, len or length - don't worry about variable name length as you'll usually end up minifying your code if you're writing anything substantial.

That said

var type, current;

for (var i = 0, length = arr.length; i < length; i++) {
    current = arr[i];
    type = typeof current;
    if (type === 'string') {
        text = current;
    }
    else if(Array.isArray(current)) { // @RobertoBonvallet's observation
       child = current;
    }
    else if (type === 'object') {
        attr = current;}
    }
}
share|improve this answer
    
Confusingly enough, in JavaScript, defining i and length inside the for loop construct doesn't limit their scope, as demonstrated by this fiddle. –  nyuszika7h Mar 5 at 17:03
    
Javascript has function scope –  megawac Mar 5 at 17:16
    
That's irrelevant. –  nyuszika7h Mar 5 at 17:24
    
All that fiddle is pointing out is that JS has function level scope –  megawac Mar 5 at 17:30
    
What it points out is that i defined inside the for loop's construct is also defined outside of the loop, which is not case in at least some other programming languages, such as C. –  nyuszika7h Mar 5 at 17:35

+1 to @Jerry and three minor notes:

  1. var i = 0, x = arr.length;
    

    I'd put the variable declarations to separate lines. From Code Complete 2nd Edition, p759:

    With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn’t have to dip into each and every line just because a single line might contain two statements.

  2. I would use longer variable names than x. Longer names would make the code more readable since readers don't have to decode the abbreviations every time when they write/maintain the code and don't have to guess which abbreviation the author uses.

    So, arrayLength would be a readable name here.

    (Clean Code by Robert C. Martin, Avoid Mental Mapping, p25)

  3. I'd have started the refactoring with creating a local variable for the result of typeof arr[i] === 'object'. It's used twice.

    (Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

share|improve this answer
2  
You are completely correct, thanks for pointing out the above. FWIW, I do adhere to the coding guidlines as noted in point #1, but for the sake of this example I was just trying to keep it to the least number of lines since it was already longer than it needed to be. As for point #3, I actually removed the 2nd instance of Array.isArray() since it wasn't needed, per @Prinzhorn's suggestion. –  DondeEstaMiCulo Mar 4 at 22:53
    
I disagree with using arrayLength for the length, I think it's unnecessarily long. I'd go for l, len, or length, unless the code is more complex. –  nyuszika7h Mar 5 at 16:51

I'm going to suggest something a bit different.

Lets step back and look at what you are trying to do. You seem to want to extract the elements of the array according to their type.

Now your code will only find the last element in the array that is of the given type. If there are two strings in the array then the first one will be ignored. This may of course be what you want, but it doesn't provide for a good reusable solution. Sometimes you may want the first in the list, other times all of them.

I would suggest creating a reusable groupBy function

function groupBy(arr, fn) {
  return arr.reduce(function(result, item) {
     var group = fn(item);
     result[group] = result[group] || [];
     result[group].push(item);
     return result;
  }, {});
}

Then you can use this function to group your array by type :

function getType(item) {
    var type = typeof item,
        isArray = Array.isArray(item);

    return (type === 'object' && !isArray) ? 'obj' :
           (type === 'string') ? 'string' : 
           (type === 'object' && isArray) ? 'array' :
             'none';
}

var result = groupBy(arr, getType); 

You can then access the types you need through the object. If you want the first item in the list of that particular type :

text = result.string && result.string[0];
child = result.array && result.array[0];
attr = result.obj && result.obj[0];

If you want the last item :

text = result.string && result.string[result.string.length - 1];
child = result.array && result.array[result.child.length - 1];
attr = result.obj && result.obj[result.attr.length - 1];
share|improve this answer
1  
I was going to write something along these lines when I saw your answer. The fact that the code as written only returns the "last of each type" really bothered me too. This seems like a big improvement. –  Floris Mar 4 at 21:28
    
The reason why I wrote it the way I did is because the format of the array (in my case) will always be in order of: [attr, text, child], and will only include, at max, 3 values. The loop only serves one purpose inside a function which pulls out some other information from the original array that is passed as a parameter. I realize that without any context it's hard to know what my intentions are. +1 for the excellent advice none-the-less. –  DondeEstaMiCulo Mar 5 at 0:37
    
@DondeEstaMiCulo I see, couldn't you just do attr=arr[0]; text=arr[1]; child=arr[2] in that case? –  Mongus Pong Mar 5 at 16:02
    
@MongusPong If the array was always [object, string, array] then yes, but the values are optional, so it could be something like [object, array] or [string] or [array], etc. –  DondeEstaMiCulo Mar 5 at 21:31
    
@DondeEstaMiCulo It does feel quite fragile to be relying on the type to identify the value. Supposing a change came along that meant there were two different strings in the array. What does the code that populates this array do? Is there no way that could create an object with the data already correctly identified rather than an array? –  Mongus Pong Mar 5 at 22:52

By using the prototype name string instead of the typeof function we can get a more accurate check of the variable type. For instance:

variable["__proto__"]["constructor"]["name"]

var Array = []; //returns "Array"
var Object = {}; //returns "Object"
var Array = new Array(); // returns "Array"

With this we no longer have to have an additional logical statement to check if our our objects are arrays in disguise or actually an object.

Using a reverse while loop means that array.length doesn't have to be evaluated each increment and reduces character wastage. It does make the logic more confusing to write in controlled sequential cases, but in this case is fine

var i=arr.length;
while(i--){switch(arr[i].__proto__.constructor.name;){
        case'Object':{attr = arr[i]; break;}
        case'String':{text = arr[i]; break;}
        case'Array':{child = arr[i]; break;}
        default:{break;}}}

or a more readable version:

var myArray = [150,"Entry",{"x":15,"y":26}]; //input array
var index = myArray.length; //evaluate the length of the array

while(index--) //decrement the index while it's greater than 0
{
   var type = myArray[index]["__proto__"]["constructor"]["name"];//what type is this variable
   switch(type)
   {
      case:'Object': //if object
      {
         attr = myArray[index]; //set attribute to field
         break;
      }
      case:'String': //if string
      {
         text = myArray[index]; //set text to field
         break;
      }
      case:'Array': //if array
      {
         child = myArray[index]; //set child to field
         break;
      }
      default:
      {
         console.log("Error, entry does not match requested types");
         break;
      }
   }
}

Using a quasi-referance object array to start with would be a lot more obvious way to do this though, it seems a rather backwards way to extract relevant information from an unsorted array than to just pick items out of your choosing.

var myArray = 
{
   "values":[150,160,170],
   "name":"entry",
   "coord":{"x":15,"y":26}
}

//then

attr = myArray["coord"]; 
text = myArray["name"]; 
child = myArray["values"];

//or

attr = myArray.coord; 
text = myArray.name; 
child = myArray.values; 
share|improve this answer
    
If I do a reverse while loop using (index--), won't it exclude the 0 index?? I kinda need that. ;) –  DondeEstaMiCulo Mar 5 at 0:43
    
@DondeEstaMiCulo nope see stackoverflow.com/questions/6867876/javascript-i-vs-i –  megawac Mar 5 at 1:53
    
@megawac Oh yeah, thanks for pointing that out. I was thinking about --index. D'oh! –  DondeEstaMiCulo Mar 5 at 2:23
1  
it performs the decrement before the function, which means it'll perform index[0] and you dont have to subtract one from the array length. Quite handy really :) –  bhiqa Mar 5 at 9:23

It seems to me a switch statement would work here.

for (i=0;arr[i];i++) {
  switch(typeof arr[i]{
    case 'string':
      text = arr[i];
    break;
   case 'object':
     if (Array.isArray(arr[i])) {
      child = arr[i];
     } else {
       attr = arr[i];
     }
    break;
  }
}

This construct of for initializes i once, checks if there is an element arr[i] in the while section and sets the incrementation.

Switch checks the type of arr[i] only once. This seems to me to be succinct and will probably prove to be faster.

share|improve this answer
1  
You have some syntax errors –  megawac Mar 4 at 13:43
    
Despite the syntax errors, I still get the meaning. I had actually thought of that last night after making this post. ;) –  DondeEstaMiCulo Mar 4 at 23:11

Here is how I would do it. I'm trading a little efficiency for a lot of readability.

for (var i = 0; i < arr.length; i++) {
    var thisItem = arr[i];
    var thisType = typeof thisItem;
    if (thisType == 'string') { text = thisItem; }
    if (thisType == 'object' && Array.isArray(thisItem)) { child = thisItem; }
    if (thisType == 'object' && !Array.isArray(thisItem)) { attr = thisItem; }
}

It would be more efficient to declare variables outside the loop. Also, one if, else if conditional statement would evaluate less conditional statements as opposed to listing them out individually like I did here. Also, declaring thisItem and thisType in the loop is not needed but does help clarify what is going on and makes the following statements more readable. In my opinion, this approach is more succinct and much easier to read than the other solutions. Also, a for loop makes more sense here than a while loop.

However, after some thought I agree with @bhiqa that an object should be used in this situation. In most cases if you're using typeof then your code can be written better. But I also realize that's not what the op asked for.

share|improve this answer
2  
Welcome to Code Review! Could you explain exactly what you're trading in term of efficiency ? In my opinion, a review should explain why we are changing the op code, so that the op can choose the "better" option or learn a new concept by your answer. –  Marc-Andre Mar 5 at 17:55
    
I wrote a 'more succinct' solution. It would be more efficient to declare variables outside the loop. Also, one if, else if conditional statement would evaluate less conditional statements as opposed to listing them out individually like I did here. Also, declaring thisItem and thisType in the loop is not needed but does help clarify what is going on and makes the following statements more readable. In my opinion, this approach is more succinct and much easier to read than the other solutions. Also, a for loop makes more sense here than a while loop. –  rvarcher Mar 5 at 20:16
1  
I don't disagree with you at all, and you should add this to your answer since this is exactly what I wanted from you : a good explanation of what you were doing! At Code Review we want to the review the code of the op to make him better, not just the code! Adding explanation is a good way to help the OP and clarify why he could choose your solution over an other one! –  Marc-Andre Mar 5 at 20:31
up vote 0 down vote accepted

I have to say, everyone really had some great suggestions. So I ended up using something from everyone's answers... I think for my particular situation, the following loop is best:

// arr = [{'id':12}, "Test", [{'id':42}, "Another Test"]];

var attr,
    child,
    i,
    len = arr.length,
    text;

for (i = 0; i < len; i++) {
    var curVal = arr[i];

    switch (typeof curVal) {
        case 'string':
            text = curVal;
            break;
        case 'object':
            if (Array.isArray(curVal)) {
                child = curVal;
            }
            else {
                attr = curVal;
            }
            break;
    }
}


Just a couple notes:

  • I almost always use for loops, especially since I almost always forget to increment my counter in a while loop, lol.
  • I ended up using a switch statement because I might add a boolean to the array, and I think it makes it easier to read (IMHO).
  • I upvoted everyone whose suggestions I used. Thanks for all of your input!
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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