2
\$\begingroup\$

Are there better ways of modularizing the code? Does it contain things JavaScript experts would avoid? Is it idiomatic? Is there a better way of signaling an empty queue than returning null?

function makeQueue () {
  // types
  let Node = {
    next : null,
    data : null  
  };

  // data
  let head = null;
  let size = 0;

  // methods
  function enqueue(x) {
    let newNode = {...Node, next : head, data : x};
    head = newNode;
    size++;
  }

  function dequeue() {
    if (size === 0) {
      return null;
    }

    let x = head.data;
    head = head.next;
    size--;

    return x;
  }

  function getSize() {
    return size;
  };

  // public
  return {
    enqueue : enqueue,
    dequeue : dequeue,
    getSize : getSize
  };
};
\$\endgroup\$
3
\$\begingroup\$

Some observations in addition to the other answer:

  • inconsistent space between function name and () - use eslint or similar to prevent typos
  • Node is used as a template for extra properties, not as a type, so I doubt its name should be capitalized as though it's a class. Also, it's effectively unused in your code.
  • const in JS is not for named constants only, but for any value that's assigned just once:

    The reason is simple, it's much easier to reason about code when it is dead obvious what can change and what cannot change. [...]

    Going back and changing a const to a let is dead simple. And going const by default makes you think twice before doing so. And this is in many cases a good thing.

    How many bugs have you seen that involved variables changing unexpectedly? I'd guess a lot. I know that the majority of bugs that I see involve unexpected state changes. You won't get rid of all of these bugs by liberally using const, but you will get rid of a lot of them!

    + if you use a JavaScript IDE, it'll warn about accidental reassignment of a const identifier.

  • A separate assignment statement with newNode doesn't seem warranted as the code would be just as obvious with a direct assignment to head.
  • getSize is too verbose. I suggest exposing it as size just like the read-only size getter in Map and Set.
  • Since you're using ES2015, also use object literal shorthand:

    return {
      enqueue,
      dequeue,
      size: getSize
    };
    
\$\endgroup\$
2
\$\begingroup\$

I'll reply to your questions in a random order, hope it's okay.

Is there a better way of signaling an empty queue than returning null?

Returning null is okay-ish for as long as the consuming will not need to store null as a valid value. IMO, there are two better ways. Which one to choose is up to you.

I am personally a big fan of defensive programming and fail fast approach. Thus, I'd prefer to throw new Error('Queue is empty -- nothing to dequeue.') so that the consuming code breaks instead of hiding a bug. I say "bug", because when a queue is empty, nobody should not even try to dequeue() from it.

If you disagree, you can at least make your class consistent with JavaScript. Return the same thing as the one returned by an empty array when pop() is called (which is undefined):

pop on empty array


Is it idiomatic?

I think, it is. The only thing that confuses me is this:

let newNode = {...Node, next : head, data : x};
               ^^^^^^^

Not sure why this line can not be simplified to let newNode = { next: head, data: x}; Similarly, I don't see much value in Node "type" if it's not really used as a type anywhere in the code.


Are there better ways of modularizing the code?

It looks good to me.


Does it contain things JavaScript experts would avoid?

I'm not an "expert", so leaving this part for someone else to answer.

\$\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.