5
\$\begingroup\$

I have implemented a very basic TODO List with no database or backend support. Though I have coded in C++ and Python before, this is my first HTML and JavaScript code. Please review the code and let me know where it can be improved.

var numericalID = 1
function addNewTask() {
var container = document.getElementById("taskContainer")

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)
    //alert(idSplitArray)
    if(this.checked) {
       labelTask.style.setProperty("text-decoration", "line-through");
    } else 
    {
       labelTask.style.setProperty("text-decoration","none");
    }})

var label = document.createElement('label')
var labelStr="label:"+numericalID
label.id=labelStr
label.htmlFor = "id";
label.appendChild(document.createTextNode(document.getElementById('taskName').value +"                  "));

var delButton=document.createElement('input')
delButton.type="image"
delButton.src="delButton.png"
delButton.id="del:"+numericalID
delButton.addEventListener('click',function(){
    var idSplitArray=this.id.split(':')
    var checkBoxId="id:"+idSplitArray[1]
    var labelStrId="label:"+idSplitArray[1]
    var delButton="del:"+idSplitArray[1]
    var lineBreakId="line:"+idSplitArray[1]
    console.log(lineBreakId)
    console.log(checkBoxId)
    console.log(labelStrId)
    console.log(delButton)
    document.getElementById(checkBoxId).remove();
    document.getElementById(labelStrId).remove();
    document.getElementById(delButton).remove();
    document.getElementById(lineBreakId).remove();


})

container.appendChild(checkbox);
container.appendChild(label);
container.appendChild(delButton)

var linebreak=document.createElement("br")
linebreakID="line:"+numericalID
linebreak.id=linebreakID
numericalID+=1
container.appendChild(linebreak)
}
<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <title>To Do</title>
      <script type="text/javascript" src="ToDo.js"></script>
  </head>
  <body align="center">
  <form action="">
  Task Name: <input type="text" id="taskName" name="fname"><br>
  <input type="submit"  value="Submit" onclick="addNewTask() ; return false;">
      <div id="taskContainer">
      </div>
</form>
  </body>
</html>

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

About the base HTML

  • It’s a good practice to add the language of the page content to the html element:

    <html lang="en">
    
  • The type attribute is not needed if the script element is used for JavaScript:

    <script src="ToDo.js"></script>
    
  • The align attribute is no longer allowed:

    <body>
    

    Use CSS instead (e.g., text-align).

  • You should use a label element for the task input:

    <label for="taskName">Task Name:</label> <input type="text" id="taskName" name="fname">
    

    This allows users to click the label to focus the input, and it allows accessibility tools to connect the label with the input, so that their users can know what the input is for.

  • The action attribute is not allowed to have an empty value. In your case, you could simply omit the attribute:

    <form>
    

About the generated HTML

  • The label elements for the checkboxes have the wrong for value. They all have for="id" instead of for="id:1", for="id:2" etc.

  • Each image button needs an alt attribute:

    <input type="image" src="delButton.png" alt="Delete" id="del:…">
    
\$\endgroup\$
5
\$\begingroup\$

I have several comments about styling:

  1. 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.

  2. 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.

  3. 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.

  4. 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.

  5. 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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