Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

There must be a better way to write this, maybe with a switch statement (which for some reason I can't get to work). Basically, if one of the conditionals is true then the class in the statement is executed. So, in this case, one or any number of combinations could be true.

if (stepOneEdited) {
    $(stepOne).addClass("complete");
}
if (stepTwoEdited) {
    $(stepTwo).addClass("complete");
}
if (stepThreeEdited) {
    $(stepThree).addClass("complete");
}
if (stepFourEdited) {
    $(stepFour).addClass("complete");
if (stepFiveEdited) {
    $(stepFive).addClass("complete");
    $(stepSix).addClass("complete");
}
share|improve this question
3  
did you forget the closing brace of if (stepFourEdited) { ? –  janos Aug 24 at 8:23
3  
Some more context would be helpful here. I suspect the system’s design is the problem; not how your conditional logic is formed. You only really have one line of logic, and the rest is duplication. When you write the same line several times, and when you start using numbers for identifiers, you need to recognise the code smell and revise your design. –  Jezen Thomas Aug 24 at 11:02

2 Answers 2

up vote 4 down vote accepted

I like @janos solution, but if that is not possible for you, you might extract the code into a function. I know, it's just one line of code, but I think it would still be beneficial:

// marks className element as complete if isEdit is true
function addComplete(isEdit, className) {
    if (isEdit) {
        $(className).addClass("complete");
    }
}

addComplete(stepOneEdited, stepOne);
addComplete(stepTwoEdited, stepTwo);
addComplete(stepThreeEdited, stepThree);
addComplete(stepFourEdited, stepFour);
addComplete(stepFiveEdited, stepFive);
// stepSix depends on stepFiveEdited, not stepSixEdited
addComplete(stepFiveEdited, stepSix);

This way, each additional step only takes one line instead of three, I think that this makes it a lot more readable (and also adding a new case is a lot easier). The other benefit is that it is easier to adapt this code if the specification changes. For example if the class name changes, or if instead of adding a class, the program should react differently, you only need to change it in one place.

share|improve this answer
2  
you could use addComplete(stepFiveEdited, setSix); rather than the if statement. –  DFord Aug 24 at 13:52
    
@DFord great catch, thanks. I updated my answer. –  tim Aug 24 at 19:44

If these conditions are independent from each other (as they seem), then there's no way to rewrite them better, and definitely not with a switch:

if (stepOneEdited) {
    $(stepOne).addClass("complete");
}
if (stepTwoEdited) {
    $(stepTwo).addClass("complete");
}
if (stepThreeEdited) {
    $(stepThree).addClass("complete");
}

What you can do is, instead of these conditions, use events, somehow. For example, given this kind of HTML:

<body>
    <p><input class="field"/></p>
    <p><input class="field"/></p>
    <p><input class="field"/></p>
</body>

If you want to add complete class to these fields when changed, you could attach a change event handler to all of them with one move:

$('.field').change(function() {
    $(this).addClass('complete');
});
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.