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.

I recently got into a discussion about the best way to set a couple of default values. I created two sample blocks that would have the same results. The "isDefined" function is specifically for ColdFusion, but another condition would work just as well.

I use BLOCK ONE. I tend to set defaults and change them only if conditions are met. It seems that I can leave out redundant code by using BLOCK ONE.

A co-worker says that BLOCK ONE is harder to read and will execute more slowly (but doesn't know why). He prefers BLOCK TWO.

Is there any particular reason to use one block or another?

BLOCK ONE

// DEFAULT VALUES
Var1 = "A";
Var2 = "B"

// MARKET ID EXISTS
if (isDefined("SomeVar")) {
    if (SomeVar eq 1) {
        Var1 = "D";
        Var2 = "E"
    } else if (SomeVar eq 2) {
        Var1 = "D";
    } else if (SomeVar eq 3) {
        Var2 = "E"
    } else if (SomeVar eq 4) {
        Var2 = "R"
    } else if (SomeVar eq 5) {
        Var1 = "Q"
        Var2 = "R"
    }
}

BLOCK TWO

// MARKET ID EXISTS
if (isDefined("SomeVar")) {
    if (SomeVar eq 1) {
        Var1 = "D";
        Var2 = "E"
    } else if (SomeVar eq 2) {
        Var1 = "D";
        Var2 = "B"
    } else if (SomeVar eq 3) {
        Var1 = "A";
        Var2 = "E"
    } else if (SomeVar eq 4) {
        Var1 = "A";
        Var2 = "R"
    } else if (SomeVar eq 5) {
        Var1 = "Q"
        Var2 = "R"
    } else {
        // DEFAULT VALUES
        Var1 = "A";
        Var2 = "B"
    }
} else {
    // DEFAULT VALUES
    Var1 = "A";
    Var2 = "B"
}

share|improve this question

closed as off-topic by 200_success Aug 26 at 18:45

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

1  
Isn't premature optimization the root of all evil in programming? I would prefer the one that's better readable to me and not per se to somebody else. –  Kid Diamond Aug 26 at 17:16
    
@KidDiamond I would rather be readable to the majority of programmers than to me, because I may not be the next one to maintain the code. But premature optimization is indeed something to avoid. –  Marc-Andre Aug 26 at 17:28
1  
@Marc-Andre That's why when you work in a team, you'd usually discuss some type of conventions that everyone agrees to follow before jumping into the project. Compromises will always have to be made, but it would be for the greater good. –  Kid Diamond Aug 26 at 17:31
1  
Identifiers such as SomeVar, Var1, and Var2 make this hypothetical code, which is off-topic for Code Review. –  200_success Aug 26 at 18:47
2  
I have reviewed the comments here, and deleted those comments that were not seeking/providing clarification on the question, but were commenting on why the question was closed. Comments are not the right place for these discusions. If you feel like this question should be discussed then post a meta question. If you have the reputation to cast a reopen vote, then feel free to do that too. If not, then feel free to answer or ask questions to earn that reputation, you need 500. –  rolfl Aug 28 at 21:18

3 Answers 3

You could get rid of the if-statements at all.

What are your configuration possibilities?

Var1 = "A";
Var2 = "B"

Is your default configuration. Dependend on several conditions you alter one or the other.

if (isDefined("SomeVar")) {
    if (SomeVar eq 1) {
        Var1 = "D";
        Var2 = "E"
    } else if (SomeVar eq 2) {
        Var1 = "D";
    } else if (SomeVar eq 3) {
        Var2 = "E"
    } else if (SomeVar eq 4) {
        Var2 = "R"
    } else if (SomeVar eq 5) {
        Var1 = "Q"
        Var2 = "R"
    }
}

So you have the following pairs:

1:['D','E']
2:['D','B']
3:['A','E']
4:['A','R']
5:['Q','R']

Why not simply write a function?

function getConfiguration(someValue){
    var conf={1:['D','E'], 2:['D','B'], 3:['A','E'], 4:['A','R'], 5:['Q','R']};
    result=conf[someValue];
    return (typeof result === 'undefined')?['A','B']: result;
}

Which is for me more readable.

share|improve this answer
    
You could also consider a ternary there –  konijn Aug 26 at 18:05
    
@konijn thanks :] –  Thomas Junk Aug 26 at 18:07
    
This is a good option: as well as the optimisations you mention, it also separates the logic from the "data" a lot more cleanly. You could get rid of the intermediary value in that as well. Also, as @Evik James is a CFML dev, for the CFML solution, the null-coalescing operator might be able to short-circuit the logic further: I dunno if it would work with associative array notation though. –  Adam Cameron Aug 26 at 18:31

As far as speed is concerned I think it depends on the execution path taken. I believe BLOCK ONE will be faster when SomeVar is not defined and when SomeVar is not equal to any of your predefined values. I believe BLOCK TWO will be faster when SomeVar is equal to any of your predefined values. But again, we are talking milliseconds here. For example.

BLOCK ONE faster

BLOCK ONE - faster when SomeVar is not defined:

  1. the variables will be set
  2. the if (isDefined("SomeVar")) condition will be checked
  3. execution ends

BLOCK TWO

  1. the if (isDefined("SomeVar")) condition will be checked
  2. the else condition will be checked
  3. the variables will be set
  4. execution ends

BLOCK ONE - faster when SomeVar is not equal to the predefined values:

  1. the variables will be set
  2. the if (isDefined("SomeVar")) condition will be checked
  3. the if (SomeVar eq 1) condition will be checked
  4. the else if (SomeVar eq 2) condition will be checked
  5. the else if (SomeVar eq 3) condition will be checked
  6. the else if (SomeVar eq 4) condition will be checked
  7. the else if (SomeVar eq 5) condition will be checked
  8. execution ends

BLOCK TWO

  1. the if (isDefined("SomeVar")) condition will be checked
  2. the if (SomeVar eq 1) condition will be checked
  3. the else if (SomeVar eq 2) condition will be checked
  4. the else if (SomeVar eq 3) condition will be checked
  5. the else if (SomeVar eq 4) condition will be checked
  6. the else if (SomeVar eq 5) condition will be checked
  7. the else condition will be checked
  8. the variables will be set
  9. execution ends

BLOCK TWO faster

BLOCK ONE

  1. the variables will be set
  2. the if (isDefined("SomeVar")) condition will be checked
  3. the if (SomeVar eq 1) condition will be checked
  4. the else if (SomeVar eq 2) condition will be checked
  5. the else if (SomeVar eq 3) condition will be checked
  6. the variables will be set
  7. execution ends

BLOCK TWO - faster when SomeVar is equal to any of the predefined values:

  1. the if (isDefined("SomeVar")) condition will be checked
  2. the if (SomeVar eq 1) condition will be checked
  3. the else if (SomeVar eq 2) condition will be checked
  4. the else if (SomeVar eq 3) condition will be checked
  5. the variables will be set
  6. execution ends

In general I think one could also argue that a switch/case statement would execute faster than deeply nested if/else statements. Again, milliseconds here.

Specific to ColdFusion, you could also gain speed by scoping the variables rather than letting ColdFusion search through each of them. There might actually be larger speed gains here.

Also specific to ColdFusion, I have seen many answers on StackOverflow recommending the use of StructKeyExists() rather than IsDefined().

Having said all of that, I have actually coded both ways as well. Lately I have been favoring the latter approach; BLOCK TWO.

share|improve this answer

Don't worry about performance with code like this: it's micro-optimisation, as well as very very much being premature optimsation.

Write your code to be as clean as possible, with as few things for the next developer to remember as they track down your code as possible.

I think your first example is closer to ideal here.

Firstly, it sets the scene: no matter what, var1 and var2 will end up with a value. This is not clear from the second block, when starting at the top. It |is clear from the first block.

Secondly, with the overly long if() block, yer both keeping the person reading the code guessing. Just do this:

Var1 = "A";
Var2 = "B"

if (!isDefined("SomeVar")) {
    exit;
}

If someVar doesn't exist: all the rest of the code is irrelevant. Let the person know. exit' here is pseudo-code. This should be in a function, so it should be a return statement. But if it's in a CFM, then actuallyexit` is just fine.

Next... if you're using a if/else if construct, you're implying that there are going to be different conditions, which is not the case here. It's one condition checking multiple values. Use a switch. Again, this pre-warns the person reading the code what's going on, and minimises what they need to remember.

Var1 = "A";
Var2 = "B"

if (isDefined("SomeVar")) {
    exit;
}

switch (SomeVar){
    case 1:
        Var1 = "D";
        Var2 = "E"
    exit;
    case 2:
        Var1 = "D";
    exit;
    case 3:
        Var2 = "E"
    exit;
    case 4:
        Var2 = "R"
    exit;
    case 5:
        Var1 = "Q"
        Var2 = "R"
    exit;
    // but what if it equals 6?
}

Notice here I'm using exit rather than break. As before, this is pseudo code: all I mean is "you're done processing, don't much about leaving the reader guessing: tell them you're done.

I recommend to you and all the commenters / answerers here that you read "Clean Code".

share|improve this answer
    
Adam, thanks for the tip on the book. I just ordered it from Amazon. –  Evik James Aug 27 at 18:50

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