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, 2014 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, 2014 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, 2014 at 18:39

2 Answers 2

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, 2011 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, 2011 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, 2011 at 23:46
  • \$\begingroup\$ @James - Yes, unfortunately JS is more forgiving than that. :) \$\endgroup\$ Jul 10, 2011 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, 2011 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, 2012 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, 2012 at 22:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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