Is there a way to shorten this code?

Question

Is there a shorter way (cleaner way really) to do the following code of mine?

if (whichToCheck == 1) {
    if ($('#input_3').val().length) {
        $('#error1').css('display', 'none');
        $('#error1').css('visibility', 'hidden');
        hasErrors = false;
    } else {
        $('#error1').css('display', 'block');
        $('#error1').css('visibility', 'visible');
        hasErrors = true;
    }
}
else if (whichToCheck == 2) {
    if ($('#input_4').val().length) {
        $('#error2').css('display', 'none');
        $('#error2').css('visibility', 'hidden');
        hasErrors = false;
    } else {
        $('#error2').css('display', 'block');
        $('#error2').css('visibility', 'visible');
        hasErrors = true;
    }
}
else if (whichToCheck == 3) {
    if ($('#input_5').val().length) {
        $('#error3').css('display', 'none');
        $('#error3').css('visibility', 'hidden');
        hasErrors = false;
    } else {
        $('#error3').css('display', 'block');
        $('#error3').css('visibility', 'visible');
        hasErrors = true;
    }
}
else if (whichToCheck == 4) {
    if ($('#input_7_0').is(':checked')) {
        $('#error4').css('display', 'none');
        $('#error4').css('visibility', 'hidden');
        hasErrors = false;
    } else {
        $('#error4').css('display', 'block');
        $('#error4').css('visibility', 'visible');
        hasErrors = true;
    }
}
else if (whichToCheck == 5) {
    if ($('#input_6').val().length) {
        $('#error5').css('display', 'none');
        $('#error5').css('visibility', 'hidden');
        hasErrors = false;
    } else {
        $('#error5').css('display', 'block');
        $('#error5').css('visibility', 'visible');
        hasErrors = true;
    }
}

Thanks!

There's no good reason to set `display:none` AND `visibility:hidden`, especially if you're going to undo both at the same time. — Blazemonger, Sep 17 '12 at 13:43
Agreed on the below (switch block and set CSS) but also all the common properties can be set once and the changes once per block — ericosg, Sep 17 '12 at 13:44
What does your markup look like? Can you get the input corresponding to the error elements based on relations in the DOM? — Jørgen, Sep 17 '12 at 13:51
Can't believe that people down vote a legit question. :0/ **Whomever down voted... post here and and convince me why it was needed.** — StealthRT, Sep 17 '12 at 14:27
I didn't down vote, I even answered, but I shouldn't have. Like zzzzBov said. — Armel Larcier, Sep 17 '12 at 15:32

Armel Larcier · Answer 1 · 2012-09-17T15:34:16.753

3

First of all you should use a switch block and store the elements you want to modify in variables. Short version:

var input, error;
switch(whichToCheck){
    case 1:
        input=$('#input_3');
        error=$('#error1');
        break;
    case 2:
        input=$('#input_4');
        error=$('#error2');
        break;
    default:
        ...
        break;
}
var hasErrors = !!input.val().length;
if(hasErrors) error.show(); else error.hide();

Or shorter for the end:

hasErrors&&error.show();
hasErrors||error.hide();

edited Sep 17 '12 at 15:34

answered Sep 17 '12 at 13:42

Armel Larcier

15,747
7
68
89

`hasErrors&&error.show()` is the kind of coding style that makes it completely unreadable and almost impossible to maintain – wroniasty Sep 17 '12 at 14:14
That's production code, not developement. And I disagree. – Armel Larcier Sep 17 '12 at 15:21
Do you mean that development code may be unreadable? – wroniasty Sep 17 '12 at 15:22
No ;-) I just don't think this specific syntax "makes code completely unreadable..." – Armel Larcier Sep 17 '12 at 15:31
ok maybe not "completely", just "slightly" :) – wroniasty Sep 17 '12 at 15:33
Edited my adjective from "better" to "shorter". Least I can do! – Armel Larcier Sep 17 '12 at 15:37

score 2 · Answer 2 · answered Sep 17 '12 at 13:43

A much better way to do these if..then...else statements would be to use a switch statement. Especially when there are multiple values that you are looking for...

switch(whichToCheck){
  case "1":
    // when whichToCheck == 1
  break;
  case "2":
    // when whichToCheck == 2
  break;
  default:
    // when the value of whichToCheck doesn't match any expected value
  break;
}

Another thing you can do to lessen the code you have is to use jQuery's show() and hide() functions to...well... hide and show elements :) You don't really have to explicitly set the display and visibility properties.

Reference -

score 2 · Answer 3 · edited May 23 '17 at 12:21

2

You can shorten a lot by selecting the elements dynamically. Also, put them in "cache" variables instead of recreating jQuery instances. And you can use a short form for jQuery's .css() method by passing in an object. Also, you should put those two different styles into variables instead of repeating them - do everything to make the code more DRY.

In a one-liner:

$('#error'+whichToCheck).css( (hasErrors = !$('#input_'+(2+whichToCheck)).val().length)
   ? {display: 'block', visibility: 'visible'}
   : {display: 'none', visibility: 'hidden'}
);

However, your ids seem to be not too regular, so I recommend a mapper as in (the possible duplicate) Alternative to a million IF statements:

var toCheck = document.getElementById( 'input_' + {1:'3', 2:'4', 3:'5', 4:'7_0', 5:'6'}[whichToCheck] ),
    errorEl = $('#error'+whichToCheck);
hasErrors = !(toCheck.type=="checkbox" ? toCheck.checked : toCheck.value);
if (hasErrors)
    errorEl.css({display: 'block', visibility: 'visible'});
else
    errorEl.css({display: 'none', visibility: 'hidden'});

Also, you don't need to set display and visibility, and if you'd use jQuery's .hide()/.show() it would work for non-block-elements, too:

errorEl[hasErrors ? "show" : "hide"]();

edited May 23 '17 at 12:21

Community

1
1

answered Sep 17 '12 at 13:47

Bergi

630,263
148
957
1,375

Did you notice the checkbox as well in there? **if ($('#input_7_0').is(':checked')) {** – StealthRT Sep 17 '12 at 13:57
Thanks for the hint, I've adapted the code. – Bergi Sep 17 '12 at 14:05
New code does not seem to work now for the input boxes. Keeps saying true when theres nothing in the boxes. – StealthRT Sep 17 '12 at 14:14
Isn't it supposed to set `hasErrors=true` if the value is the empty string (== has no length)? – Bergi Sep 17 '12 at 14:43
Still has it as fine without having anything in the boxes. Your original code **hasErrors = !toCheck.val().length;** did just find for the boxes but not for the checkbox. – StealthRT Sep 17 '12 at 15:49
Um, did you change the jQuery-`$` to `document.getElementById`? Code for jQuery-elements would be like `$toCheck.is(":checkbox") ? $toCheck.prop("checked") : $toCheck.val()` – Bergi Sep 17 '12 at 16:14
I copied just like you had it above in your code. – StealthRT Sep 17 '12 at 16:29

score 1 · Answer 4 · answered Sep 17 '12 at 13:42

1

This one is better to set CSS:

$('#error1').css({'display': 'none', 'visibility': 'hidden'});

answered Sep 17 '12 at 13:42

keaukraine

5,315
29
54

Thanks for the short way of setting the CSS, keaukraine! – StealthRT Sep 17 '12 at 14:18

score 1 · Answer 5 · answered Sep 17 '12 at 13:43

1

For starters, you could create css classes and use the addClass function in jQuery

Examaple:

CSS

.hide{
  display: none;
  visibility: hidden;
}

jQuery

$('#error1').addClass("hide");

answered Sep 17 '12 at 13:43

Chase

29,019
1
49
48

I've thought about doing that but still just wanted to use just one CSS class. – StealthRT Sep 17 '12 at 14:19
You could use `hide` class and set it when appropriate, and instead of having a new class you could use the `removeClass` function that jQuery also offers. – Chase Sep 17 '12 at 14:28

score 1 · Answer 6 · edited Sep 17 '12 at 13:45

1

As you wrote your code

 $('#error3').css('display', 'block');
 $('#error3').css('visibility', 'visible');

Repeats at several places for style change. Can you make it into a function such that you pass the error field name.

For #error3 you pass the param as 3, then the display and visibility params.

Thanks

edited Sep 17 '12 at 13:45

Bojangles

99,427
50
170
208

answered Sep 17 '12 at 13:44

Abhi

6,471
6
40
57

Jørgen · Answer 7 · 2012-09-17T14:21:18.263

1

You could generate the ids based on the value of whichToCheck and use jQuery's hide method to hide the elements:

function toggleErrors($input, whichToCheck){
  if($input.val() !== '' || $input.is(':checked')){
    $('#error' + whichToCheck).hide();
  }else{
    $('#error' + whichToCheck).show();
  }
}

Pass in whichToCheck and the corresponding input element to the function.

edited Sep 17 '12 at 14:21

answered Sep 17 '12 at 13:45

Jørgen

8,820
9
47
67

This is correct but i also have a checkbox value in this as well. – StealthRT Sep 17 '12 at 14:20
I added the checked condition which is a bit of a hack. However, I'd encourage you to use relations in order to connect the inputs and error elements if possible. – Jørgen Sep 17 '12 at 14:23
What does **input** represent? should it be **this.value**? – StealthRT Sep 17 '12 at 14:27
You need to pass in the input to the function. If it is invoked from an event handler, you could use this.value. – Jørgen Sep 17 '12 at 14:30
You'll need to find the value of input using a switch like the other answers suggests, unless you can find a connection between inputs and errors other ways...such as DOM relations, but I can't help you with that as I don't know how your mark up looks like. – Jørgen Sep 17 '12 at 14:34

score 1 · Answer 8 · answered Sep 17 '12 at 13:49

Use Switch() statements as mentioned By Armel.
Extract Show() & Hide() methods separately.

Here is the sample:

if (whichToCheck == 1) {
        if ($('#input_3').val().length) {
            Show('#error1');
            hasErrors = false;
        } else {
            Hide('#error1');
            hasErrors = true;
        }
    } else if (whichToCheck == 2) {
        if ($('#input_4').val().length) {
            Show('#error2');
            hasErrors = false;
        } else {
            Hide('#error2');
            hasErrors = true;
        }
    } else if (whichToCheck == 3) {
        if ($('#input_5').val().length) {
            Show('#error3');
            hasErrors = false;
        } else {
            Hide('#error3');
            hasErrors = true;
        }
    } else if (whichToCheck == 4) {
        if ($('#input_7_0').is(':checked')) {
            Show('#error4');
            hasErrors = false;
        } else {
            Hide('#error4');
            hasErrors = true;
        }
    } else if (whichToCheck == 5) {
        if ($('#input_6').val().length) {
            Show('#error5');
            hasErrors = false;
        } else {
            Hide('#error5');
            hasErrors = true;
        }
    }

    function Show(id) {

        $(id).show();

    }

    function Hide(id) {
        $(id).hide();
    }

Here, I have mentioned to use Switch assuming that the person knows how to use. So, I didin explain howto use Switch. But I wanted to point out to move some sphagetti coding for Hide & Show to separate methods to take care of them. It was the context in my explanation and not 'use of Switch'. — Dhanasekar Murugesan, Sep 20 '12 at 05:54

score 1 · Answer 9 · answered Sep 17 '12 at 13:50

The key to shortening your code seems to be the source of the whichToCheck variable. Alternatively you can do this:

//...
if ( $('#input_' + whichToCheck).val().length ) {
    $('#error_' + whichToCheck).css ( { 'display' : 'none', 'visibility' : 'hidden' } );
} else {
    $('#error_' + whichToCheck).css ( { 'display' : 'block', 'visibility' : 'visible' } );        
}

Is there a way to shorten this code?

9 Answers9