current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 1 down vote favorite

I have written the code below and I am trying to work out if there is a more efficient way of doing it: i.e. less lines of code and quicker etc.

I am also wondering whether it is OK to declare variables inside of an if ... else if statement.

function test() {
    var x = 3;
    if (x > 5) {
        var msg = "m", state = "d";
    } else if (x < 5) {
        var msg = "a", state = "d";
    } else {
        var msg = "e", state = "n";
    }
    $('#id1').removeClass().addClass(state);
    $('#id2').html("Some text " + msg + " and more text.");
    $('#id3').attr('title', "Different text " + msg + " still different text.");
}

As background, the following code is the original code I had before refactoring/rewriting:

function test() {
  var x = 3;
  if (x > 5) {
        $('#id1').removeClass().addClass('d');
        $('#id2').html("Some text message and more text.");
        $('#id3').attr('title', "Different text message still different text.");
  } else if (x < 5) {
        $('#id1').removeClass().addClass('d');
        $('#id2').html("Some text message2 and more text.");
        $('#id3').attr('title', "Different text message2 still different text.");
  } else {
        $('#id1').removeClass().addClass('n');
        $('#id2').html("Some text message3 and more text.");
        $('#id3').attr('title', "Different text message3 still different text.");
  }
}
share|improve this question
1  
Just had a 'collaboration' with Jamal. There is a lot of history on this question, and you're right, the code has not significantly changed, and your edits added more context. To be clear though, if this question was new, it would be closed as 'example code', and 'not real'. I'll restore the question with a few small modifications. –  rolfl♦ Jul 3 at 15:16
    
Thanks @rolfl. Just for the record, the code was 'real' code I used in a project. I'd simply reduced it down to the most relevant code for clarity and the benefit of those answering the question, so that it would be easy to read and didn't go on for lines and lines. –  tw16 Jul 3 at 18:22
    
You're welcome, but, to be clear, your code, as it stands, can be reduced further because x is always 3 ... because the code makes no sense.... it's example code ;-) –  rolfl♦ Jul 3 at 18:39
add comment

2 Answers

up vote 4 down vote accepted

I don't know what the purpose of declaring your variable within your if statement is supposed to be here. For readability, I would declare those variables at the beginning of your function, like so:

function test() {
    var x = 3, 
        msg, 
        state;
    if (x > 5) {
        msg = "m", state = "d";
    } else if (x < 5) {
        msg = "a", state = "d";
    } else {
        msg = "e", state = "n";
    }
    $('#id1').removeClass().addClass(state);
    $('#id2').html("Some text " + msg + " and more text.");
    $('#id3').attr('title', "Different text " + msg + " still different text.");
}

That being said, I don't know a reason why you couldn't do so; it most depends on readability and the best practices you are following.

share|improve this answer
    
The idea behind it was simple. I wanted the variable to be different based on the different scenarios. So I thought I should just re/define the variable in each scenario. –  tw16 Jul 10 '11 at 23:40
    
But... How you declare that variable is not important. You'll see that the variable is declared anyways. –  Jared Farrish Jul 10 '11 at 23:43
    
You don't want to be defining your msg variable inside the if scope because strictly speaking it shouldn't be accessible outside the if. Unfortunaely javascript allows us to write code like this, but its not a good idea. –  James Khoury Jul 10 '11 at 23:46
    
@James - Yes, unfortunately JS is more forgiving than that. :) –  Jared Farrish Jul 10 '11 at 23:57
1  
@tw16 - Think about this for a second. If your question is whether or not your value should be set within or before an if statement, I think you have just answered your own question - it should be declare your variable and then set it accordingly. While the alternative is certainly possible, the best practice is to declare the (known) variables and then set them before working on them later in your script. –  Jared Farrish Jul 11 '11 at 0:05
show 12 more comments
up vote 0 down vote

admittedly readability usually takes a second seat to speed for me (I know a lot of you won't like that), but for me I would probably use some inline logic for smaller, faster code:

function test(){
    var x=3;
    $('#id1').removeClass().addClass((x==5?'n':'d'));
    $('#id2').html("Some text " + (x<5?'a':(x>5?'m':'e')) + " and more text.");
    $('#id3').attr('title', "Different text " + (x<5?'a':(x>5?'m':'e')) + " still different text.");
}
share|improve this answer
1  
there is certainly a lot less of your code than in the other examples. But I think your code is not easily readable or flexible. I loathe encountering code like this. It's just my preference. –  Evik James Jun 12 '12 at 21:03
    
Like I said, I know a lot of you wouldn't like this, but I develop social networks almost exclusively and in my experience speed trumps readability as long as you're experienced enough to understand the code you write –  Trey Jun 12 '12 at 22:40
add comment

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.