1
\$\begingroup\$

I'd highly appreciate it if you can:

  • Spot flaws, especially bad practices and suggest a possible alternative method.
  • Spot inefficiencies and suggest how to perform that task efficiently.
  • You may review my HTML/CSS as well.

var addRecordFormVisibility = false;
var addRecordForm = document.getElementById('addRecord');
var addRecordFormButton = document.getElementById('submit');
var errors = document.getElementById('errors');
var addRecordFormTrigger = document.getElementById('addRecordFormTrigger');

function prepare() {
    addRecordForm.style.display="none";
}

function triggerAddRecordForm() {
    if(addRecordFormVisibility === false) {
        addRecordForm.style.display="block";
        addRecordFormVisibility = true;
    } else {
        addRecordForm.style.display="none";
        addRecordFormVisibility = false;
    }
}

function submit(event) {
    var userInput = document.getElementById('description').value;

    errors.innerHTML = '';

    if(userInput.length === 0) {
        errors.innerHTML = "Please fill in the form!";
    } else {
        var listItem = document.createElement('li');
        listItem.innerHTML = userInput;

        document.getElementById('todoList').appendChild(listItem);
    }

    event.preventDefault();
}

document.addEventListener("DOMContentLoaded", prepare, false);
addRecordFormButton.addEventListener("click", submit, false);
addRecordFormTrigger.addEventListener("click", triggerAddRecordForm, false);
#todoApplication {
    width: 800px;
    height: 100%;
    margin: 0 auto;
    background: #EEEEEE;
    border: 1px solid #DDDDDD;
    border-radius: 5px;
    padding: 10px;
}
#errors {
    color: red;
}
<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="UTF-8">
        <title>JavaScript</title>
        <link rel="stylesheet" href="src/style.css">
    </head>
    <body>
        <div id="todoApplication">
            <button id="addRecordFormTrigger">Trigger Add Record Form</button>
            <div id="errors">

            </div>
            <form id="addRecord" method="POST">
                <input id="description" type="text" name="description" placeholder="Enter the task here">

                <button id="submit">Submit</button>
            </form>
            <ul id="todoList"></ul>
        </div>

        <script src="src/TodoApp.js"></script>
    </body>
</html>

\$\endgroup\$
2
  • \$\begingroup\$ Seeing what it does, I think "trigger" should be "toggle" instead ;-) \$\endgroup\$ Jun 27, 2015 at 14:51
  • \$\begingroup\$ Yeah. I forgot that word. \$\endgroup\$ Jun 28, 2015 at 10:01

1 Answer 1

2
\$\begingroup\$

Ok so, your code is overall looking nice, but a few changes could be made:


It's important to make sure you leave whitespace around your operators, like:

addRecordForm.style.display="none";
                           ^^

Could use whitespace around the operators.


I try not to suggest ternaries very often, but I believe that triggerAddRecordForm could be simplified with the use of one.

function triggerAddRecordForm() {
    if(addRecordFormVisibility === false) {
        addRecordForm.style.display="block";
        addRecordFormVisibility = true;
    } else {
        addRecordForm.style.display="none";
        addRecordFormVisibility = false;
    }
}

into:

function triggerAddRecordForm() {
    addRecordForm.style.display = addRecordFormVisiblity ? "none" : "block";
    addRecordFormVisibility = !addRecordFormVisibility;
}

On the subject of triggerAddRecordForm(), in general you shouldn't compare to false, then else it. You should leave the check empty (checks if the variable is true) like so:

if(addRecordFormVisibility) {
    doStuff();
} else {
    doOtherStuff();
}

In submit(), you could improve errors.innerHTML = ''.
Why don't you just set errors to document.getElementById('errors').innerHTML instead, seeing as you don't use errors other than for that?

In submit() you should also improve your line spacing as well, as it seems a little strange.

\$\endgroup\$
0

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.