2
\$\begingroup\$

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.");
  }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – rolfl
    Jul 3 '14 at 15:16
  • \$\begingroup\$ 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. \$\endgroup\$
    – tw16
    Jul 3 '14 at 18:22
  • \$\begingroup\$ 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 ;-) \$\endgroup\$
    – rolfl
    Jul 3 '14 at 18:39
4
\$\begingroup\$

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.

\$\endgroup\$
17
  • \$\begingroup\$ 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. \$\endgroup\$
    – tw16
    Jul 10 '11 at 23:40
  • \$\begingroup\$ But... How you declare that variable is not important. You'll see that the variable is declared anyways. \$\endgroup\$ Jul 10 '11 at 23:43
  • \$\begingroup\$ 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. \$\endgroup\$ Jul 10 '11 at 23:46
  • \$\begingroup\$ @James - Yes, unfortunately JS is more forgiving than that. :) \$\endgroup\$ Jul 10 '11 at 23:57
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ Jul 11 '11 at 0:05
0
\$\begingroup\$

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.");
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Evik James
    Jun 12 '12 at 21:03
  • \$\begingroup\$ 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 \$\endgroup\$
    – Trey
    Jun 12 '12 at 22:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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