Checking on a condition in loop

Question

I'm trying to create a function that gets all elements by id, one-by-one and compares their id with their className.

If one of them is different, the element id'50' src e set for X, else (if they are all equal) the element's src e set to Y.

The thing is, if element id'50' == class'50', it sets src automatically to Y, even though some elements have different id and class.

It appears to be testing only the last element instead of testing all.

function checking(){
  for(var i = 1; i < 51; i++){
    if(document.getElementById(''+i+'').id != document.getElementById(''+i+'').className){
      document.getElementById('50').src = '../images/X.gif';
      complete = false;
    }
    else{
      document.getElementById('50').src = '../images/Y.jpg';
      complete = true;
    }   
  }
}

Only simple Javascript.

Any suggestion are welcome!

Also don't use CSS class names that start with a number. See http://stackoverflow.com/questions/448981/what-characters-are-valid-in-css-class-names-selectors — Tomalak, Jun 27 '15 at 16:12
Apart from that it's completely nonsensical to have elements that have the same class and ID. Don't do that. — Tomalak, Jun 27 '15 at 16:13
IDs and classes can start with a number in HTML5. There is no such thing as "CSS class names". It's true that CSS identifiers can't start with a number, but it can be escaped. — Oriol, Jun 27 '15 at 16:14
@Oriol Of course there is such a thing as ["CSS class names"](http://www.w3.org/TR/CSS21/syndata.html#value-def-identifier). — Tomalak, Jun 27 '15 at 16:26

score 1 · Accepted Answer · 2015-06-27T16:26:24.307

Only the last one is tested (or that's how it seems) because you never stop the loop. So naturally, it continues till the end and complete is the last value set.

If you want to stop when setting it to false, then use break or even return since it seems you're done at that point.

And FYI, this is pretty pointless...

document.getElementById(''+i+'').id

It's the same as doing i or technically i.toString().

So rewriting your code, we could do this instead:

function checking(){
  for(var i = 1; i < 51; i++){
    if(i != document.getElementById(i).className) {
       document.getElementById('50').src = '../images/X.gif';
       complete = false;
       return;
    }  
  }

  document.getElementById('50').src = '../images/Y.jpg';
  complete = true;
}

Here I got rid of the else and put its code down below the loop. Now if there's a non-match, we set complete and return. If no non-matches are found, it goes to the code after the loop when the loop is done.

It does however seem like you're abusing variables. We don't know where complete is, but it seems like you should just return the desired boolean.

function checking(){
  for(var i = 1; i < 51; i++){
    if(i != document.getElementById(i).className) {
       document.getElementById('50').src = '../images/X.gif';
       return false;
    }  
  }

  document.getElementById('50').src = '../images/Y.jpg';
  return true;
}

var complete = checking();

Or a cleaner approach would be to create a single selector for all the IDs and use .every().

var sel = "#" + Array.apply(null, 50).map(function(_, i) { return i+1; }).join(",#");

var complete = [].every.call(document.querySelectorAll(sel), function(el) {
    return el.id === el.className;
});

document.getElementById('50').src = '../images/' + (complete ? 'Y.jpg' : 'X.jpg');

score 1 · Answer 2 · answered Jun 27 '15 at 16:35

You can set the src attribute before entering the loop, and only change it if the id and className attributes don't match. You also need need to break or return once you've found a mismatch:

function checking() {
  var fifty = document.getElementById('50');
  fifty.src = '../images/Y.jpg';
  for (var i = 1; i < 51; i++) {
    var e = document.getElementById(i);
    if (e.id != e.className) {
      fifty.src = '../images/X.gif';
      return;
    }
  }
}

Checking on a condition in loop

2 Answers2