Commonmark migration

edited Jun 10, 2020 at 13:24

Community Bot

• 1

Inline Side-by-side Side-by-side Markdown

deleted 9 characters in body

edited Sep 3, 2015 at 22:55

SirPython

• 13.4k
• 3
• 38
• 93

Inline Side-by-side Side-by-side Markdown

answered Sep 3, 2015 at 22:47

SirPython

• 13.4k
• 3
• 38
• 93

#HTML

###ID

In HTML, the property values should always be encased in ""s, even if the type is not necessarily intended to be a string. If needed, the JavaScript can parse the value into an actual number type.

In your HTML, you did not put ""s around the id. You should.

<button style="height:150px;width:150px" id="7" onclick="...

---

##Inline styling

Inline styling in bad practice in HTML:

<button style="height:150px;width:150px"

In fact, there is another problem this: you are repeating the same style attribute for each and every button.

I recommend creating a CSS file and creating a rule for all buttons that sets these properties:

button {
 height: 150px;
 width: 150px;
}

---

#JavaScript

In your color_single_button, you are accessing whatever element has the id property of id two times in a row. The document.getElementById function is not very efficient, because it has to search the entire DOM. Your code would be a lot faster if you stored this value in a variable:

var button = document.getElementById(id);
var number = button.innerHTML;
...

---

In fact, let's take this one step further: let's store all the buttons in an array.

This is very easy to do with document.getElementsByTagName. This will return an array of all the elements that are of the specified tag:

var buttons = document.getElementsByTagName("button");

Then, instead of accessing the element by passing it's ID to getElementById, you use the ID - 1 as an indexer in this array to access an element.

This will significantly boost the efficiency of your code.

Note: I am not very good with HTML, but it may not be good practice to use the id in this case, but rather a custom data- property.

---

#Questions

I put HTML and JavaScript together. Would separating them improve readability?

I would recommend moving the JavaScript to an external file. It will aid maintainability in the future.

---

I generate 9 buttons by hand, and I copy-paste the size for each one. Can I avoid so much repetition?

Yes, you could, and there are two ways that I recommend

• AngularJS

AngularJS can make generating multiple, slightly changing elements very easy using ng-repeat.

• Vanilla JavaScript

In this method, you would use a for-loop and document.createElement to create a bunch of button elements and append them to the body of the code. I'm not sure if this is the best choice, though, as it is generally discouraged to generate HTML from JavaScript.

However, if you were to try this, it might look like this:

document.onload = {
 for(var i = 0; i < BUTTON_COUNT; i++) {
 var button = document.createElement("button");
 button.style = "...";
 button.id = i;
 button.onclick = "switch_with_neighbor(" + id + ")";
 document.body.appendChild(button);
 }
}

---

I am confused by the looping constructs of JavaScript. Is my use of the for .. in sensible?

Yes, I think it is sensible: JavaScript's for/in loops are meant for looping through object constructs. However, I would not say:

var i in nears

Generally, in JavaScript, the singular version of the object being looped through is used as the indexing variable:

var near in nears

---

I put the neighbours hash inside a function. Is this standard practice? Is this weird?

I don't really know if this has to do with practice, but it sure is weird: this object is the exact same thing every time.

It would make a lot more sense to put this outside of the function so it is loaded once and only once, rather than it being created and destroyed every time the function starts and stops.

---

I cast variables to different types all the time. Can you help me spot unnecessary conversions?

Yes, there are a few spots where you are unnecessarily converting types.

nears = neightbors[parseInt(current, 10)];

In JavaScript, you can use a string to access the property of an object. For example, look at this:

var obj = {
 foo: "bar"
}

I could access it like this:

obj.foo

And:

obj["foo"]

You can apply this to your circumstance: current is already a string, so there is no point in calling parseInt.

var number = parseInt(document.getElementById(parseInt(id)).innerHTML);

The method document.getElementById is looking for a string parameter (but, of course, can take a number parameter). Therefore, you are just fine leaving out the parseInt(id)