HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Onscreen keyboard

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
keyboardonscreenstackoverflow

Problem

My code below is for a sort of onscreen keyboard. I was just wondering if it could be written shorter with a lot of ifs and elses.

function input(key) {
    fieldName   = currentSide+'_scratchfield';
    field       = document.getElementById(fieldName);

    del         = "DELETE";
    if(key == 'sp') key = ' ';

    if(key == 'minus')  {
        if(field.value == del) {
            return false;
        } else if(field.value.charAt(0) == "-") {
            field.value = field.value.substr(1, field.value.length);
            return false;
        } else {
            field.value = "-"+field.value;
            return false;
        }
    }
    if(key == 'clr')    {   
        if(field.value == "" || field.value == null) {
            return false;
        } else if (field.value == del) {
            field.value = "";
            return false;
        } else {
            field.value = field.value.substring(0, field.value.length-1); 
            return false;
        }
    }
    if(key == 'del')    {   
        if(field.value == "" || field.value == null) {
            key = del;
        } else if(field.value == del) {
            field.value = "";
            return false;
        } else {
            field.value = ""; 
            key = del;
        }
    }

key = key.toUpperCase();
if(field.value == del) { field.value = ""; }
field.value += key;
}

Solution

I am not sure about getting it shorter, but it could be improved.

  • Indentation is a bit of a problem, check out http://jsbeautifier.org/ and paste your code to see what I mean



  • Use var to declare your variables!



  • If you return in an if block, then you don't need an else if block, just if will do



  • Your code needs more comment, if(key == 'sp')



  • I wish we had more code so that we could give a more insightful review



  • You can replace if(field.value == "" || field.value == null) { with if(!field.value)



  • You can skip the curly braces of an if, but you should not skip the newlines.



All in all, that makes something like this:

function input(key) {
    var fieldName = currentSide + '_scratchfield',
        field     = document.getElementById(fieldName),
        del       = "DELETE";
    if(key == 'sp')
        key = ' ';
    if(key == 'minus'){
        if(field.value == del)
            return false;
        if(field.value.charAt(0) == "-") {
            field.value = field.value.substr(1, field.value.length);
            return false;
        }
        field.value = "-"+field.value;
        return false;
    }
    if(key == 'clr'){
        if(!field.value)
            return false;
        if (field.value == del){
            field.value = "";
            return false;
        }
        field.value = field.value.substring(0, field.value.length-1);
        return false;
    }
    if(key == 'del'){
        if(!field.value)
            key = del;
        if(field.value == del) {
            field.value = "";
            return false;
        }
        field.value = "";
        key = del;
    }
    key = key.toUpperCase();
    if(field.value == del)
        field.value = "";
    field.value += key;
}

Code Snippets

function input(key) {
    var fieldName = currentSide + '_scratchfield',
        field     = document.getElementById(fieldName),
        del       = "DELETE";
    if(key == 'sp')
        key = ' ';
    if(key == 'minus'){
        if(field.value == del)
            return false;
        if(field.value.charAt(0) == "-") {
            field.value = field.value.substr(1, field.value.length);
            return false;
        }
        field.value = "-"+field.value;
        return false;
    }
    if(key == 'clr'){
        if(!field.value)
            return false;
        if (field.value == del){
            field.value = "";
            return false;
        }
        field.value = field.value.substring(0, field.value.length-1);
        return false;
    }
    if(key == 'del'){
        if(!field.value)
            key = del;
        if(field.value == del) {
            field.value = "";
            return false;
        }
        field.value = "";
        key = del;
    }
    key = key.toUpperCase();
    if(field.value == del)
        field.value = "";
    field.value += key;
}

Context

StackExchange Code Review Q#44619, answer score: 5

Revisions (0)

No revisions yet.