mwittig / node-milight-promise

A node module to control Milight LED bulbs and OEM equivalents such as Rocket LED, Limitless LED Applamp, Easybulb, s`luce, iLight, iBulb, and Kreuzer
MIT License
114 stars 27 forks source link

Queue #31

Closed sanderkoenders closed 6 years ago

sanderkoenders commented 6 years ago

I've been inspecting the code and I think I figured out how it works. The _synchronize function is building up some kind of queue with promises right? Why do you use this instead of creating an actual queue that s fetches commands when the previous one is done? This would make the code way more readable.

So something like:

const queue = [];
let active = false;

function sendCommand(cmd) {
  queue.push(cmd);

  if(!active) {  
    setActive(true);
  }
}

function setActive(bool) {
  active = bool;

  if(active) {
    console.log("Activated");

    processCommands();
  } else {
    console.log("Deactivated");
  }
}

function processCommands() {
  if(queue.length > 0) {
    let cmd = queue.shift();

    processCommand(cmd).then(processCommands)
  }
  else {
    setActive(false);
  }
}

function processCommand(cmd) {
  return new Promise((resolve, reject) => {
    setTimeout(function() {
      console.log(cmd);

      resolve();
    }, 100);
  });
}

sendCommand(1);
sendCommand(2);
sendCommand(3);
sendCommand(4);

setTimeout(function() {
  sendCommand(5);
}, 1000);

Fiddle here: https://jsfiddle.net/wgogdb12/

mwittig commented 6 years ago

Hi. Thank you very much for looking into node-milight-promise and your feedback!

The basic rationale behind using synchronized promises rather than a message queue is to have more flexibility with respect to setting the synchronization points in the code. Actually, _synchronize is used for the socket initialialization (am I ready to send?), the sendCommand, the Socket Closure (close if there are no pending promises), and it is optionally used to synchronize multiple Milight instances.

Needless to say the code has grown over time and things could be simplified for sure using an approach similiar to what you're suggesting. At a frist glance, I'd prefer a queue of functions (or promises), however. I need to look into this in more detail, however, to decide on the best approach. Some refactoring is on my todo list list anyway also to modernize the code with respect to the advancements of Javascript.

mwittig commented 6 years ago

I am closing this now as the question has been answered. I'll revisit the matter at a later stage as part of the refactoring work