Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queue #31

Closed
sanderkoenders opened this issue Oct 12, 2017 · 2 comments
Closed

Queue #31

sanderkoenders opened this issue Oct 12, 2017 · 2 comments
Labels

Comments

@sanderkoenders
Copy link

sanderkoenders commented Oct 12, 2017

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
Copy link
Owner

mwittig commented Oct 12, 2017

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
Copy link
Owner

mwittig commented Oct 19, 2017

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

@mwittig mwittig closed this as completed Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants