I have several comments about styling:
check the indentation and fix it. There are some different styles that can be observed but universally lines that are inside a function (or even more generally - a scope) would have one indentation level more than before.
Spacing - be consistent. In one line you have checkbox.value = "value";
with spaces around the equals sign. The next line you have var idStr="id:"+numericalID
- no spaces around the equals or the operator. I personally put spaces around each but that's personal preference. You can have spaces only around equals signs, or even only on the right side of equals signs or only around operators, etc. Whatever you pick - be consistent.
Semicolons at the end of the line. In JavaScript, they are not strictly required due to a feature called automatic semicolon insertion (ASI). So, a lot of times you can get away without putting them. Although occasionally, you may find your code doesn't behave as expected. Still, as long as you understand how ASI works, you should be fine. Just pick a style - with or without a semicolon and stick with it. I personally prefer putting them in because I find the ASI rules quite annoying to try and remember every time.
Opening curly brackets - they should be either at the same line as the statement they accompany or the next line. You're pretty consistent with having it on the same line aside from where the else
shows up.
Super tiny issue - you don't have linebreakID
declared properly. It's missing the var
keyword, so the variable is now an implied global. I'm mentioning it here with the style issues because you've been consistent with having var
statements, so it seems like a simple typo, rather than a different problem.
Styling is pretty important for readable and understandable code. You can employ a linter like ESLint or JSCS (JavaScript Code Style) where you can define the style you want (indentation using spaces/tabs, spaces around equals signs, etc) and then they will scan your code and report back any style violations. They even have an autofix option that takes care of all the style changes, so you don't have to, for example, manually go and fill in every missing semicolon in the file. Linters are a pretty useful tool.
Other than styles, I'd say the function is quite long. It can be broken down into smaller ones for the sake of readability. This is subjective, I know, but having to scroll to read everything is usually too much. There is no duplicate code but you can still just do something like this:
function createCheckbox() {
var checkbox = document.createElement('input');
checkbox.type = "checkbox";
checkbox.name = "name";
checkbox.value = "value";
var idStr = "id:" + numericalID;
checkbox.id = idStr;
checkbox.addEventListener('change', function() {
var idSplitArray = this.id.split(':');
var labelStr = "label:" + idSplitArray[1];
var labelTask = document.getElementById(labelStr);
if (this.checked) {
labelTask.style.setProperty("text-decoration", "line-through");
} else {
labelTask.style.setProperty("text-decoration", "none");
}
});
return checkbox;
}
So you don't have all the boring details about setting up a checkbox around more important code. It's a judgement call how you'd split off the code, though - you might elect to have the event handler defined back in the addNewTask
function or even split off into a separate function. Whatever it is, the point is to make the main function shorter by splitting off logical portions of the code. In effect, you get code comments - only instead of //creating checkbox
you have checkbox = createCheckbox()
. But the latter is also self-documenting and has less of a chance to drift away from the code and become an irrelevant and/or inaccurate comment.
For actual logic improvements - right now, there is string manipulation to determine which HTML ID corresponds to which element ID by employing string splitting and conversely creating complex ID values to accommodate that. That works and it's honestly something even big name libraries out there do. Still, it's possible to do better - you can use data-*
attributes in HTML. In short, you can have custom attributes attached to HTML elements. The custom attribute is prefixed with data-
and you can define the rest of the name. So, you can add a data-id="1"
, for example, to all elements that will operate on the element with ID 1
. That way, you don't need to do string manipulation to get the correct data out of it, you can directly reference the custom attribute.
And that's most of my advice. I think there are other things, too but I don't consider them that important. I'm honestly quite pleased with this code and my main issue is really formatting inconsistency. I am especially pleased that you create the HTML elements programmatically via document.createElement
followed by setting the attributes. That is a very good style and amazing to see in a beginner. I very often see even experienced JS developers opting to just do something like parentElement.innerHTML = '<input type="checkbox" id="id:"' + numericalID +'" />
which can be quite annoying to read and has some potentially disadvantages, some even potential security vulnerabilities.
Overall, I'd applaud this effort, especially from a beginner.