Shorten my JS & HTML Code

Question

I have some code here that I would like to shorten, how would I be able to do that? Should I create a loop? Should I create a function with a loop inside that keeps adding '1' each time? There are 3 groups of code who's lines are the same except for the numbers. Please, I really need an answer:

function checkit(){
var radio1img1 = document.getElementById("radio1img1");
var radio1img2 = document.getElementById("radio1img2");
var radio1img3 = document.getElementById("radio1img3");
var radio1img4 = document.getElementById("radio1img4");
var radio1img5 = document.getElementById("radio1img5");
var radio1img6 = document.getElementById("radio1img6");
var radio1img7 = document.getElementById("radio1img7");
var radio1img8 = document.getElementById("radio1img8");
var radio1img9 = document.getElementById("radio1img9");

if (radio1img1.checked){
    changeImage('img1','http://i.imgur.com/QAUUxYF.jpg');
} else {
    changeImage('img1','http://i.imgur.com/RcuPIGF.png');
}
if (radio1img2.checked){
    changeImage('img2','http://i.imgur.com/QAUUxYF.jpg');
} else {
    changeImage('img2','http://i.imgur.com/RcuPIGF.png');
}
if (radio1img3.checked){
    changeImage('img3','http://i.imgur.com/QAUUxYF.jpg');
} else {
    changeImage('img3','http://i.imgur.com/RcuPIGF.png');
}
if (radio1img4.checked){
    changeImage('img4','http://i.imgur.com/QAUUxYF.jpg');
} else {
    changeImage('img4','http://i.imgur.com/RcuPIGF.png');
}
if (radio1img5.checked){
    changeImage('img5','http://i.imgur.com/QAUUxYF.jpg');
} else {
    changeImage('img5','http://i.imgur.com/RcuPIGF.png');
}
if (radio1img6.checked){
    changeImage('img6','http://i.imgur.com/QAUUxYF.jpg');
} else {
    changeImage('img6','http://i.imgur.com/RcuPIGF.png');
}
if (radio1img7.checked){
    changeImage('img7','http://i.imgur.com/QAUUxYF.jpg');
} else {
    changeImage('img7','http://i.imgur.com/RcuPIGF.png');
}
if (radio1img8.checked){
    changeImage('img8','http://i.imgur.com/QAUUxYF.jpg');
} else {
    changeImage('img8','http://i.imgur.com/RcuPIGF.png');
}
if (radio1img9.checked){
    changeImage('img9','http://i.imgur.com/QAUUxYF.jpg');
} else {
    changeImage('img9','http://i.imgur.com/RcuPIGF.png');
}
}

<table border="2">
<tr>
<td align="center"><b>B1</b></td>
<td>
<label>
<input onchange="checkit();" type="radio" name="radio1" id="radio1img1">
<img align="center" name="radio1" class="theimage" id="img1" height="45px" width="45px" src="http://i.imgur.com/DGofFGc.png">
</input>
</label>
 </td>
<td>
<label>
<input onchange="checkit();" type="radio" name="radio1" id="radio1img2">
<img align="center" name="radio1" class="theimage" id="img2" height="45px" width="45px" src="http://i.imgur.com/DGofFGc.png">
</input>
</label>
 </td>
<td>
<label>
<input onchange="checkit();" type="radio" name="radio1" id="radio1img3">
<img align="center" name="radio1" class="theimage" id="img3" height="45px" width="45px" src="http://i.imgur.com/DGofFGc.png">
</input>
</label>
 </td>
<td>
<label>
<input onchange="checkit();" type="radio" name="radio1" id="radio1img4">
<img align="center" name="radio1" class="theimage" id="img4" height="45px" width="45px" src="http://i.imgur.com/DGofFGc.png">
</input>
</label>
 </td>
<td>
<label>
<input onchange="checkit();" type="radio" name="radio1" id="radio1img5">
<img align="center" name="radio1" class="theimage" id="img5" height="45px" width="45px" src="http://i.imgur.com/DGofFGc.png">
</input>
</label>
 </td>
<td>
<label>
<input onchange="checkit();" type="radio" name="radio1" id="radio1img6">
<img align="center" name="radio1" class="theimage" id="img6" height="45px" width="45px" src="http://i.imgur.com/DGofFGc.png">
</input>
</label>
 </td>
<td>
<label>
<input onchange="checkit();" type="radio" name="radio1" id="radio1img7">
<img align="center" name="radio1" class="theimage" id="img7" height="45px" width="45px" src="http://i.imgur.com/DGofFGc.png">
</input>
</label>
 </td>
<td>
<label>
<input onchange="checkit();" type="radio" name="radio1" id="radio1img8">
<img align="center" name="radio1" class="theimage" id="img8" height="45px" width="45px" src="http://i.imgur.com/DGofFGc.png">
</input>
</label>
 </td>
<td>
<label>
<input onchange="checkit();" type="radio" name="radio1" id="radio1img9">
<img align="center" name="radio1" class="theimage" id="img9" height="45px" width="45px" src="http://i.imgur.com/DGofFGc.png">
</input>
</label>
 </td>
</tr>
</table>

Look at the network website codereview.stackexchange.com :-) On SO, this is off topic — Yotam Salmon, Aug 03 '16 at 13:49
Yes, make a loop. Don't shorten the HTML (though you do need to indent it) — 4castle, Aug 03 '16 at 13:51
@ Yotam Salmon What is SO? @4castle If I can't shorten it is there anything that will quickly change the numbers to the next digit so I can just copy it and paste into the next line? It's already indented just fine, I just had to add 4 spaces to all of it to make it valid here and I was too lazy to indent everything with 4 spaces so I just indented everything that had to be indented.... — Doctor Wayne, Aug 03 '16 at 13:55
Don't give every element an id. When you use `document.querySelectorAll("table input")` you can use the index number instead. SO is short for "Stack Overflow" — 4castle, Aug 03 '16 at 13:57
@4castle How would I incorporate document.querySelectorAll("table input")? Please give me an example with my code. — Doctor Wayne, Aug 03 '16 at 13:59
@Yotam Salmon I'm not sure how this is off topic this talks about code... — Doctor Wayne, Aug 03 '16 at 14:04
@DoctorWayne Even though it's not 100% off topic, it would be more appropriate to ask that on codereview.stackexchange.com rather than stackoverflow (SO) — Yotam Salmon, Aug 03 '16 at 19:10
@YotamSalmon If it's not off-topic, then it shouldn't be migrated. It is not completely off-topic on Code Review either, but a good rule of thumb is if there's an accepted answer, don't move it. [Distinguishing between SO and CR](http://meta.codereview.stackexchange.com/questions/5777/a-guide-to-code-review-for-stack-overflow-users/5778#5778) can be hard, but you get the hang of it. — Trojan404, Aug 03 '16 at 19:18
@Trojan404 1) The migration recommendation was **way** before there was even an answer to the question. 2) Based on the link you gave, this question **should** be on CR - *Answers can suggest any improvements, Code already works as intended* — Yotam Salmon, Aug 03 '16 at 19:48
@YotamSalmon Take a look at the **What are the most important considerations?** section. The first bullet point explains that just because you can doesn't mean you should. Beyond this, let's keep this comment stream on topic with the question. If you want to discuss this further, join me in [The Second Monitor](http://chat.stackexchange.com/rooms/8595/the-2nd-monitor). :) — Trojan404, Aug 03 '16 at 20:04

4castle · Accepted Answer · 2016-08-03T16:41:04.493

4

Don't use a table layout. They aren't responsive to mobile layouts, and they don't fit the elements you're displaying. If you want a table layout, use a CSS table layout.
Don't use old presentational attributes like align. That should also be handled by CSS.
Use a loop. This will also make it so you don't need an id for every element.
Use background images. This will allow your HTML to be much cleaner.

bindRadios('radio1');

function bindRadios(name) {
  var radios = document.querySelectorAll('input[name="' + name + '"]');
  for (var i = 0; i < radios.length; i++) {
    radios[i].onchange = function() { checkIt(radios) };
  }
}

function checkIt(radios) {
  for (var i = 0; i < radios.length; i++) {
    radios[i].parentNode.style.backgroundImage =
      'url(' + (
        radios[i].checked 
          ? 'http://i.imgur.com/QAUUxYF.jpg' // green check
          : 'http://i.imgur.com/RcuPIGF.png' // red X
      ) + ')';
  }
}

.table {
  display: table;
  border: 2px solid black;
}
.table > label {
  display: table-cell;
  width: 65px;
  height: 45px;
  background: url('http://i.imgur.com/DGofFGc.png')
              right / 45px 45px
              no-repeat;
}
.bold {
  font-weight: bold;
}

<div class="table">
  <div class="bold">B1</div>
  <label><input type="radio" name="radio1" /></label>
  <label><input type="radio" name="radio1" /></label>
  <label><input type="radio" name="radio1" /></label>
  <label><input type="radio" name="radio1" /></label>
  <label><input type="radio" name="radio1" /></label>
  <label><input type="radio" name="radio1" /></label>
  <label><input type="radio" name="radio1" /></label>
  <label><input type="radio" name="radio1" /></label>
  <label><input type="radio" name="radio1" /></label>
</div>

edited Aug 03 '16 at 16:41

answered Aug 03 '16 at 15:30

4castle

32,613
11
69
106

1

Okay, that looks great! How would I get a border on that though? – Doctor Wayne Aug 03 '16 at 15:49
2

Use the `border` style in CSS. – 4castle Aug 03 '16 at 15:54
1

Wait, one last question, how would I add another group of radios to the JS? – Doctor Wayne Aug 03 '16 at 16:34
2

Add another group of radios to the HTML, and then call `bindRadios` with the name. I've abstracted that for you to make it simpler. – 4castle Aug 03 '16 at 16:46
1

So all I have to do is copy the 'row', use the top and left functions in CSS to position my 'row', copy `bindRadios('radio1')` & replace 'radio1' with the name for my new 'row'? – Doctor Wayne Aug 03 '16 at 16:57
2

Yes. And with the "top and left functions in CSS" those should be default. – 4castle Aug 03 '16 at 16:59
1

Well it appears it only works in your snippet and online HTML/CSS/JS playgrounds... When I actually added it to my HTML page it didn't work anymore... Back to the drawing board I guess... It might just be my Browser or something isn't at the HTML5/CSS3 level, though I just updated my browser... – Doctor Wayne Aug 03 '16 at 18:13
2

You're probably having [this issue](http://stackoverflow.com/q/14028959/5743988). Try doing `window.onload = function(){bindRadios('radio1')};` – 4castle Aug 03 '16 at 18:24
1

Eureka! BTW your the king of editing LOL I follow what you said and you edit it LOL. – Doctor Wayne Aug 03 '16 at 18:28
2

Yeah... I usually edit too much. I'll answer, and then 10 minutes later think of another improvement. – 4castle Aug 03 '16 at 18:32
1

BTW just to let you know but you have to put the `window.onload = function(){..}` piece of code around the whole thing, otherwise only 1 of them will work. – Doctor Wayne Aug 03 '16 at 19:42

Shorten my JS & HTML Code

1 Answers1