4
\$\begingroup\$

I am working on a simple to do list and i was hoping if a experienced person could review my code in hope for feedback. I don't use any libraries as i haven't taken time to look about them. I decided to stick with plain js.

Thank you for your time.

Edit: Thank you for the reply, now i'm looking into the semantics in html and fixing my code. ;)

let i = 1;
function myFunction(){
    n = i.toString();
    let theInput = document.getElementById('input').value;
    if(theInput == ""){
         return false;
}
    else{
    let input = `<li class = list id = div${n}>` + theInput +`  <button onclick = myFunction2("div${n}")>Done</button>`;
    newDiv = document.createElement(`div${n}`);
    newDiv.innerHTML = input;
    const currentDiv = document.getElementById("div0");
    currentDiv.insertBefore(newDiv, currentDiv.nextSibling);
    i++;
    }
}

    const myFunction2 = function(id){
        let dou = document.getElementById(id)
        dou.remove();
    }


    const myFunction3 = function(className){
    let elements = document.getElementsByClassName(className);
    while(elements.length > 0){
        elements[0].parentNode.removeChild(elements[0]);
    }
}
    
    h1{ 
        font-family: Kufam;
        font-size: 50px;
        border-color: black;
        border-style: solid;
        border-width: 12px;
        border-radius: 8px;
        text-align:  center;
    }


    .list{
        font-family: Kufam;
        font-size: 25px;
        text-align: center;
    }
<!DOCTYPE html>
<html>
<title>To do List</title>
<header>
    <link href="https://fonts.googleapis.com/css?family=Kufam" rel="stylesheet" type="text/css">
    <link rel="stylesheet" type="text/css" href="todoliststyle.css">
    <script type="text/javascript" src="functionList.js"></script>
    <h1>
        <br>To do List <br>
        <input id="input" type="text" placeholder="what to do" required maxlength="50">
            <button onclick="myFunction()" >Add</button>
            <button onclick="myFunction3(`list`)">Clear All</button> <br><br>
        </h1>

    <div id="div0">

</header>
</html>

\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review. I have rolled back your last edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Mar 1 at 15:55
4
\$\begingroup\$

Your app seems to be working :) Good job and keep going ! Take a look at the below notes. I will try to add more a bit later.

  • Treat Your variable name as instructions for other developers (and yourself from future). They should be descriptive. Function with name such as Function2 are not acceptable
  • Your HTML inputs do not have associated labels
  • You use h1 in a wrong way. It's not a proper tag to place the input in. Use a div instead. H1 is used to define (only one) main heading on Your page. Read about HTML semantics and the meaning of each tag
  • some of your let variables could be changed to consts. The rule is as follows - use const whenever it's possible and if it's not - use let
  • Do not style elements by their tag names like h1{} add the proper class to the element and style it with .className syntax in Your CSS
  • Instead of an onCLick inline event listener in Your HTML You could add this handling in the js file using element.addEventListener('typeOfEvent', callback)
  • Add descriptive ID's to the elements - div0 means absolutely nothing
  • Your whole logic is in the if/else statement. Try to move it outside
  • To DO title should not be in the br tag. Use a header or a p instead
  • Your script logic could be also improved -> I will try to describe more a bit later
  • You could also use a shortcut for border styles border: 12px solid black
\$\endgroup\$
2
\$\begingroup\$

Coding style

  • Format your code with correct indent. Format your HTML with correct closing tags.
  • Name your variable / function / DOM elements with some meaningful names. Not myFunction1 nor div0.
  • You declared myFunction as global one, but use const for myFunction2. I would suggest use same style for every functions.

HTML

  • If you are posting a snippet, skip <!doctype html> and <html> would be acceptable. If you are posting whole HTML, make sure to include <head>, <body>, <meta charset="utf-8" /> tags.
  • Add lang attribute to your HTML, maybe on <html> element.
  • <h1> is used for title of page. Not for input. Use proper tags.
  • <li> should only be placed under <ul> or <ol>. It should not be placed under <div>.
  • Use common HTML tags if available. In case you need custom tags, use name with - in it. For example, <todo-list-item> is acceptable (but not preferred), but <div1> is not.
  • When you need custom tags or using <div> try adding role on it to make it more meaningful.

JavaScript

  • Save a reference to DOM elements, so you do not to getElementById every time some event triggered.
  • Never, never, never feed innerHTML with constructed string values. Especially something from user input. It may cause XSS issues.
  • Add event listeners with addEventListener instead of onclick attribute is preferred.
  • When button [Done] is clicked, you removed the <li> item but keeps its parent in DOM. It should be a mistake.

CSS

  • Use margin / padding to locate items. Do not use <br> and space character (U+0020).

Others

  • Wrap input in a <form> and listen to submit event instead of click event. So user may add the todo item with an Enter key. (Don't listen to keyup / keydown / keypress events, they may be confused with IME.)
  • Clear the input when an todo item is added successfully. So user may add next item without clean the input manually.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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