Problem Statement
Write a HTML page holding a form and a text field. Using JavaScript make the text field to accept numbers only. When a non-number character is entered through the keyboard (or by any other way), make the field red for a while and do not accept the change (preserve the previous value of the field).
Expected Output
enter image description here
Solution
<!DOCTYPE html>
<html>
<head>
<title>Text field</title>
<meta charset="UTF-8">
<script type="text/javascript">
window.onload = function(){
document.formex.numberinput.onchange = checkIfNumber;
function checkIfNumber(){
if (isNaN(parseInt(document.formex.numberinput.value, 10))){
document.formex.numberinput.innerHTML = document.formex.numberinput.value;
document.formex.numberinput.readOnly = true;
setTimeout(function(){
document.formex.numberinput.readOnly = false;
}, 10000);
}
}
}
</script>
<style type="text/css">
input[readonly] {
background-color: red;
}
</style>
</head>
<body>
<form name="formex">
<input type="text" name="numberinput">
</form>
</body>
</html>
Above code uses name attributes as element lookup approach.
Can we improve this code?
2 Answers 2
Your code doesn't follow the Single Responsibility Principle.
What do you need to do :
- Check the text change
- Verify if the input contains a number
- Potentially change the state of the input (disable/turn red the input)
- Re-enable it.
Right now, you have one function that takes care of all this, we should split it. Because, checkIfNumber shouldn't disable a textbox, that doesn't make sense! :)
You need to have functions that aren't coupled to your inputs, so we'll try to introduce parameters instead of accessing the document all the time.
function isNumber(value) {
return !(isNaN(parseInt(value, 10)));
}
Notice I could have create a isNotANumber function, but that's not good as it's confusing. Also, this function doesn't need to be in the window.onload scope, it could very well be reused anywhere else.
Next, we want to disable the input and turn it red. I wouldn't write a function only to disable it. But, writing a function to disable an element for a time span seems like a good plan :
function disableFor(input, time) {
input.readOnly = true;
setTimeout(function() {input.readOnly = false;}, time);
}
Finally we need to run the time out for 10 seconds.
window.onload = function(){
//or document.getElementsByName("numberinput")[0];
var numberInput = document.formex.numberinput;
numberInput.onChange = function() {
if(isNumber(numberInput.value)) { return; }
//I'm not sure why you do this.
numberInput.innerHTML = numberInput.value;
disableFor(numberInput, 10000);
};
}
function isNumber(value) {
return !(isNaN(parseInt(value, 10)));
}
function disableFor(input, time) {
input.readOnly = true;
setTimeout(function() {input.readOnly = false;}, time);
}
Notice that I inverted your condition to reduce nesting. So I just return if the value is a number, there's less nesting and well.. That's more readable!
-
\$\begingroup\$ For you question, I'm not sure why you do this, I do this because the problem statement says, preserve the previous value of the field \$\endgroup\$overexchange– overexchange2015年12月17日 00:24:54 +00:00Commented Dec 17, 2015 at 0:24
-
\$\begingroup\$ @overexchange Ooohhh, yeah that makes sense. Ahah. You should add a comment about it, it would be clearer that way! :) \$\endgroup\$IEatBagels– IEatBagels2015年12月17日 00:26:41 +00:00Commented Dec 17, 2015 at 0:26
-
\$\begingroup\$ I could not understand the semicolon syntax
};after callingdisableFor. It is an ending brace for callback ofonChange. \$\endgroup\$overexchange– overexchange2015年12月17日 00:31:03 +00:00Commented Dec 17, 2015 at 0:31 -
\$\begingroup\$ @overexchange Oh, maybe that semicolon isn't necessary. I was at work, couldn't test what I'm writing and I don't write Javascript that often. But I thought the review brought something interesting. :p \$\endgroup\$IEatBagels– IEatBagels2015年12月17日 00:36:28 +00:00Commented Dec 17, 2015 at 0:36
-
\$\begingroup\$ Can you test, if this
numberinputgoes readonly for exactly 10 sec? On firefox, I see the problem with this code after your suggested changes. because I could just see the blink of red color. \$\endgroup\$overexchange– overexchange2015年12月17日 00:42:00 +00:00Commented Dec 17, 2015 at 0:42
TopinFrassi has already provided a great answer. Since there is not much else to say, my review will be fairly short.
document.formex.numberpinput...
How many times are you going write something like that?
This constant property access, while it is not that big a difference, it unnecessarily slowing down your code.
Your code will look a lot cleaner if you just stick this value in a variable:
document.formex.numberinput.onchange = checkIfNumber; function checkIfNumber(){
Why is this onchange setting separate of the function creation? Just move the function up one level to the line above it and you won't have to name it. It also makes your code cleaner:
input.onchange = function() {
...
window.onload = function(){
You could completely get rid of needing to set up an event if you move this script to the very bottom of the DOM body.
I don't know how this affects performance, but I assume it would be slightly better now that it doesn't have to worry about an event.
Here is what your code now looks like:
var input = document.formex.numberinput;
input.onchange = function() {
if (isNaN(parseInt(input.value, 10))) {
input.innerHTML = input.value;
input.readOnly = true;
setTimeout(function() {
input.readOnly = false;
}, 10000);
}
}
.read-onlywould be better, then you could use it on other read-only elements, which could include any form element \$\endgroup\$