Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How can i write this code in a more efficient way. The only thing changing is the id and css property.

if($('#borderT').is(':checked')){$x.css('borderTopWidth',x)}else{$x.css('borderTopWidth','0')}
if($('#borderR').is(':checked')){$x.css('borderRightWidth',x)}else{$x.css('borderRightWidth','0')}
if($('#borderB').is(':checked')){$x.css('borderBottomWidth',x)}else{$x.css('borderBottomWidth','0')}
if($('#borderL').is(':checked')){$x.css('borderLeftWidth',x)}else{$x.css('borderLeftWidth','0')}
share|improve this question
See stackoverflow.com/q/2279373/301816 – stevebot Oct 26 '11 at 18:42
Efficient as in...pretty? – Justin Pearce Oct 26 '11 at 18:42
Pretty ugly if I say so! – marko Oct 26 '11 at 18:47

migrated from stackoverflow.com Oct 26 '11 at 18:48

This question came from our site for professional and enthusiast programmers.

3 Answers

up vote 5 down vote accepted
var sides = { T: 'Top', R: 'Right', B: 'Bottom', L: 'Left' };
for (s in sides) {
    $x.css('border' + sides[s] + 'Width', $('#border'+s).is(':checked') ? x : '0');
}

DRY as possible (without knowing more about your markup).

share|improve this answer
This is perfect. Thank you. – Pinkie Oct 30 '11 at 6:39
+1 for DRY. I call it "optimizing for future laziness". – Paperjam Nov 2 '11 at 3:25

Use the conditional operator and combine all the .css statements into a single map:

$x.css({
    'borderTopWidth':    $('#borderT').is(':checked') ? x : 0,
    'borderRightWidth':  $('#borderR').is(':checked') ? x : 0,
    'borderBottomWidth': $('#borderB').is(':checked') ? x : 0,
    'borderLeftWidth':   $('#borderL').is(':checked') ? x : 0
});

You could even move that conditional check to an external function, although that won't make the code any more efficient, just easier to read:

$x.css({
    'borderTopWidth':   getBorder('#borderT'),
    'borderRightWidth': getBorder('#borderR'),
    'borderBottomWidth':getBorder('#borderB'),
    'borderLeftWidth':  getBorder('#borderL')
});

function getBorder(id) {
    x = "5px";
    return $(id).is(':checked') ? x : 0;
};

http://jsfiddle.net/mblase75/hdpZn/1/

share|improve this answer

You can use an object as a map and using it like an array with the key as the id and value being the css property.

var map = { 
    borderT: "borderTopWidth", 
    borderR: "borderRightWidth",
    borderB: "borderBottomWidth",
    borderL: "borderLeftWidth"
}

function(id){
    if($('#' + id).is(':checked'))
    {
        $x.css(map[id],x)
    }
    else{
        $x.css(map[id],'0')
    }
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.