7
\$\begingroup\$

I'm working on building a website to help people practice English. In this practice, the answer in the blank needs to be validated on hitting enter.

I'd like to know if there is a simpler/better way to write it since I need to use it for a lot of pages.

HTML:

<p>There’s an energy here. At least, I think there is. Is it just that I know this city to be so? That there can be skiing a half hour away on the north shore mountains <input class="fillBlank" type="text" id="one" placeholder="your answer"> there is tennis in the city?</p>
<p>But then, <input class="fillBlank" type="text" id="two" placeholder="your answer"> my walk I took this photo below.</p>
<p>The city is also lush which means that it rains on <input class="fillBlank" type="text" id="three" placeholder="your answer">. Okay, maybe more than occasionally at this time of year.</p>

CSS:

.fillBlank {
width: 100px;
border-radius: 0;
border: none;
background-color: #eff9f7;
padding: 3px; 
text-align: center;
}

JS:

 $("#vancouver").ready(function() {
  $(".fillBlank").val("");
  });

// vancouver quiz answer one

var one = document.getElementById("one");
one.addEventListener("keydown", function (e) {
if (e.keyCode === 13) {
valone(e);
}
});

function valone(e) {

var one = document.getElementById("one").value;

if (one === "while") {

document.getElementById("one").style.color = "green";
}

else {
document.getElementById("one").style.color = "red";
}
}

// vancouver quiz answer two

var two = document.getElementById("two");
two.addEventListener("keydown", function (e) {
if (e.keyCode === 13) {
valtwo(e);
}
});

function valtwo(e) {

var two = document.getElementById("two").value;

if (two === "on") {

document.getElementById("two").style.color = "green";
}

else {
document.getElementById("two").style.color = "red";
}
}

// vancouver quiz answer three


var three = document.getElementById("three");
three.addEventListener("keydown", function (e) {
if (e.keyCode === 13) {
valthree(e);
}
});

function valthree(e) {

var three = document.getElementById("three").value;

if (three === "occasion") {

document.getElementById("three").style.color = "green";
}

else {
document.getElementById("three").style.color = "red";
}
}
\$\endgroup\$
6
  • 2
    \$\begingroup\$ Welcome to Code Review. I've taken the liberty to better divide your blocks of code, by their languages. I hope it helps you on future questions and answers, with codes of multiple languages. \$\endgroup\$ Feb 21, 2017 at 12:38
  • 1
    \$\begingroup\$ Create an object having mapping of ID of textbox and correct value, and bind event using jQuery on on all the elements. Demo. Will suggest to use keyup and blur, check demo \$\endgroup\$
    – Tushar
    Feb 21, 2017 at 12:43
  • 1
    \$\begingroup\$ @200_success The code works as intended, please check this demo Also, I think Also, the code doesn't work for other pages unless I attach a new js script to every new page. means to make the code generic. \$\endgroup\$
    – Tushar
    Feb 21, 2017 at 12:49
  • \$\begingroup\$ @Tushar But asking how to rewrite code using jQuery is still asking for code to be written. The answer to "Is there a more efficient way to write this code using jQuery?" is simply "Yes!". \$\endgroup\$ Feb 21, 2017 at 12:54
  • 2
    \$\begingroup\$ @200_success Since there was a mix of On and Off-Topic questions, and working code, I've edited it down to only the On-Topic ones. \$\endgroup\$
    – Kaz
    Feb 21, 2017 at 12:59

1 Answer 1

1
\$\begingroup\$

I can be rather quick with the CSS part, use indentation:

.example{
    oneTab: indented;
}

Round one

Then, you Javascript. Again, indentation. After that, I've removed several blank lines which had no purpose

Result:

 $("#vancouver").ready(function() {
    $(".fillBlank").val("");
});

// vancouver quiz answer one
var one = document.getElementById("one");
one.addEventListener("keydown", function (e) {
    if (e.keyCode === 13) {
        valone(e);
    }
});

function valone(e) {
    var one = document.getElementById("one").value;
    if (one === "while") {
        document.getElementById("one").style.color = "green";
    }
    else {
        document.getElementById("one").style.color = "red";
    }
}

// vancouver quiz answer two
var two = document.getElementById("two");
two.addEventListener("keydown", function (e) {
    if (e.keyCode === 13) {
        valtwo(e);
    }
});


/* repeat code for every question */

Round Two

Make use of the short if/else. Example below:

function valone(e) {
    var one = document.getElementById("one").value;
    //                                            Statement    ?  true   : false
    document.getElementById("one").style.color = one==="while" ? "green" : "red";
}
//---------------------------------------------------------------------
// And now save the 'one' element so we dont select it twice
function valone(e) {
    var one = document.getElementById("one");
    //                 Statement          ?  true   : false
    one.style.color = one.value==="while" ? "green" : "red";
}

Round Three

You repeat yourself for every question. This means that when you add one question, you have to update all of that. What if you add 15 questions? Imagine the huge amount of code that that would take!

So, time to fix that. All three of your functions do the same. They check an element for a specific value. They all get triggered on a keydown. Lets try to fix that to a single function:

// First select the elements we want to check the values of, you've allready classed those [fillBlank]:
$('.fillBlank').on('keydown', function(e){
    // At this point we have all the inputs on all keydown event, we only want enter:
    if (e.keyCode === 13) {
        // Now we have all the elements but only execute this on enter.
    }
});

Now comes the fun part. We want to be able to check ALL the inputs in one go, but give every input a UNIQUE value to check. For this we can use objects:

var correctAnswers = {
    one   : 'while',
    two   : 'on',
    three : 'occasion'
}
$('.fillBlank').on('keydown', function(e){ /* ... */ });

Neat little list like that right? Easy to expand aswell, just add one line et voila. We're now gonna add this to our code:

if (e.keyCode === 13) {
    // Now we have all the elements but only excecute this on enter.
    // Start checking the value for the input where the user pressed enter:
    // We can access the value via this.id because we named the input 'one', and there is a key 'one' in correctAnswers
    this.style.color = this.value=== correctAnswers[this.id] ? "green" : "red";
}

Well, thats it really. Now you can add plenty more items without building a million functions. The code below does the same thing as yours, but without hardcoding.

Result:

$("#vancouver").ready(function() {
    $(".fillBlank").val("");
});
// The correct answers:
var correctAnswers = {
    one   : 'while',
    two   : 'on',
    three : 'occasion'
}
// First select the elements we want to check the values of
$('.fillBlank').on('keydown', function(e){
    // At this point we have all the inputs on all keydown event, we only want enter:
    if (e.keyCode === 13) {
        this.style.color = this.value=== correctAnswers[this.id] ? "green" : "red";
    }
});

There are some extra things you can do to improve your code.

  • What if I type While. If you want to ignore uppercase, you can lowercase the this.value to always check the answers lowercased
  • You might want to add a check if this.id is an existing value of correctAnswers (hint: it's called .indexOf())
  • You might want to add/remove a class. CSS is for styling, not JS. This way, when you want to change the style of something correct/incorrect you do it in the stylesheet.
  • You might want to add the color names (or if you've updated the classnames) to a variable, e.g.: var outcomes = {right:'green', wrong: 'red'}; alert(outcomes.right);
\$\endgroup\$
3
  • \$\begingroup\$ One think to note here. Its better if you toggle a class such as .text-error or .text-success instead of setting the color as its more flexible with the styling. Just don't forget to set some rules in css \$\endgroup\$ Feb 21, 2017 at 15:52
  • \$\begingroup\$ Haha I was just added that as bonus excersise for the OP \$\endgroup\$
    – Martijn
    Feb 21, 2017 at 15:53
  • \$\begingroup\$ Oh my I'm embarrassed lol \$\endgroup\$ Feb 21, 2017 at 15:56

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.