Hold a text input for 10s as readonly

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



Solution

    
        Text field
        
        
            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);
                      }
                 }
            }
        
        
                input[readonly] {
                   background-color: red;
                }
        
    
    
        
            
        
    

Above code uses name attributes as element lookup approach.

Can we improve this code?

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!

StackExchange Code Review Q#114192, answer score: 7