4
\$\begingroup\$

I feel like the if-else combination isn't the most optimal and I'm also not sure if I need a loop

var Auktionator = function(){
var rennt = false;



this.versteigern = function(objekt){
        if(rennt === false){
        for(var i = 1; i<=4; i++){
            if(i === 1){
                setTimeout(function(){console.log(objekt + " zum ersten")},1000*i);
            }
            else if(i === 2){
                setTimeout(function(){console.log(objekt + " zum zweiten")},1000*i);
            }
            else if(i === 3){
                setTimeout(function(){console.log(objekt + " zum dritten")},1000*i);
            }
            else if(i === 4){
            setTimeout(function(){console.log(objekt + " verkauft!")},1000*i);

        }
         rennt = true;
    }
    }
}
}
\$\endgroup\$
  • \$\begingroup\$ I would put the words into a list and use i as the index \$\endgroup\$ – user193661 Nov 18 '15 at 8:58
5
\$\begingroup\$

Definetely not. The loop is useless (and un-optimizing) here.

As far as I can see, the index i is only used to calculate some constants' values. Realise that your original loop has the same effect than dropping off the loop and the ifs clauses, and hard-code the values computed from i:

setTimeout(function(){console.log(objekt + " zum ersten")},1000);
setTimeout(function(){console.log(objekt + " zum zweiten")},2000);
setTimeout(function(){console.log(objekt + " zum dritten")},3000);
setTimeout(function(){console.log(objekt + " verkauft!")},4000);

Or, another valid alternative could be taking advantage of the resembling of the actions and generalize them by using data:

var messages=[" zum ersten", " zum zweiten", " zum dritten", " verkauft!"];

This would be more comprehensive and flexible, since it allows to add more messages without changing the logic a little.

Update

I recommend RobH's answer, which is based upon arrays, and works fine.

\$\endgroup\$
  • \$\begingroup\$ makes alot of sense! thank you very much! \$\endgroup\$ – Max Seifert Nov 18 '15 at 10:32
  • \$\begingroup\$ Thanks, but I've realised that there is another better way. I've updated my answer. \$\endgroup\$ – Little Santi Nov 18 '15 at 14:50
  • \$\begingroup\$ @RobH No, you are wrong. My loop is zero-based: (var i=0;i<messages.length;i++), so it never reaches i==4. \$\endgroup\$ – Little Santi Nov 18 '15 at 15:49
  • 2
    \$\begingroup\$ @TopinFrassi - No. The i variable iscaptured, it's just that the loop executes to the end, setting i to 4 before the first setTimeout function is called. C# had the same behaviour for closures in a loop in .Net 4 but it was changed for .Net 4.5 \$\endgroup\$ – RobH Nov 18 '15 at 16:14
  • 1
    \$\begingroup\$ @RobH You are right. :-( I deleted my update and recommended your answer. I can't delete my own post because it was selected as best. If the OP sets if off, I would delete it. \$\endgroup\$ – Little Santi Nov 25 '15 at 9:09
4
\$\begingroup\$

Little Santi's answer has a bug because it is closing over the i variable in that asynchronous call in the for loop. Here's a simplified version of that code running in Chrome's console:

Bug in the loop

The approach is a good one, you just need to copy the value of the i variable. There are many tricks for that but you can improve readability by introducing another function displayWithDelay.

var messages=[" zum ersten", " zum zweiten", " zum dritten", " verkauft!"];
var displayWithDelay = function (message, delay) {
    window.setTimeout(function() { console.log(message); }, delay);
}

for (var i = 0; i < messages.length; i++)
{
    displayWithDelay(messages[i], 1000*(i+1));
}
\$\endgroup\$

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.