Questions about the spinner code

Here is the spinner code we got working.

Would it, or does it make sense to change this: https://jsfiddle.net/utb2154f/

  function addEvents(handlers) {
    eventHandlers.afterPlayerReady = handlers.afterPlayerReady;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

  function init(initEventHandlers) {
    addEvents(initEventHandlers);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

To this? https://jsfiddle.net/eu7bcx24/

I found it works this way also.

Is this good, not good?

I’m leaning towards it staying how it was originally written.

I don’t think It needs to be changed to this.

function addEvents(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

  function init(options) {
    addEvents(options.afterPlayerReady);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

Continuing the discussion from Setting up single-player tests before adding spinner:

At 2nd, 3rd glance.

As I understand it, it is better written this way: https://jsfiddle.net/Lkbeut57/

  function addEvents(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

  function init(options) {
    addEvents(options.afterPlayerReady);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

  return {
    addPlayer,
    init,
    play
  };
}());

videoPlayer.init({
  afterPlayerReady: function initCover() {
    manageCover.init(function playVideo() {
      videoPlayer.play();
    });
  }
});

Which would make it an improvement from how it was written before.

The addEvents function has a parameter that allows for multiple handlers, resulting in leaving room for growth as needed.

With functions, it’s important to get the naming of the function and its parameters correct. A plural function name should have parameters that allow for multiple items. A singular function name should only have singular parameters.

  // a plural function name, with a plural function parameter
  function addEvents(handlers) {
    eventHandlers.afterPlayerReady = handlers.afterPlayerReady;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

  // a singular function name, with a singular function parameter
  function addEvent(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

You however have proposed a plural function name with a singular function parameter:

  // A plural function name with a singular function parameter
  function addEvents(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

A plural function name that can only support a single function parameter is deeply flawed. At minimum, the addEvents function name should be renamed to its singular form, of addEvent instead.

  // A singular function name with a singular function parameter
  function addEvent(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

However, in this case that singular function name and parameter is also flawed. Usually a function called addEvent should let you specify which type of event. Just calling it addEvent gives you the flawed impression that it should add the event handler to an element, but that is not the case.

Instead, a more specific name for your modified function parameter structure that is more appropriate is to call it initAfterPlayerReady instead, as the function creates the custom afterPlayerReady event.

  // A more appropriate name and parameter reveals what it does
  function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

But, whether that is appropriate or not isn’t all that easy to determine without using tests in which we specify our requirements for the code.

Later on the event handler is actually attached to an element:

    const eventHandler = eventHandlers.afterPlayerReady;
    iframe.addEventListener("afterPlayerReady", eventHandler);

after which that event gets dispatched.

    iframe.dispatchEvent(events.afterPlayerReady);

So in total, no. Your proposed update is quite flawed, but if you want to remove the flexibility of being able to add other beneficial event handlers in the future, you can reduce the scope of the function to being singular in nature instead of plural.

3 Likes

if you want to remove the flexibility of being able to add other beneficial event handlers in the future, you can reduce the scope of the function to being singular in nature instead of plural.

Which means this would be used.
https://jsfiddle.net/x0ugw8q6/

Is the word options fine, or would that be changed to something else?

  function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }
  
  function init(options) {
    initAfterPlayerReady(options.afterPlayerReady);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

That’s not how the change is done though, because you may now have broken the tests that were in place.

Here’s how it gets done, without breaking anything in the tests.

The as-of-yet incomplete set of tests are at https://jsfiddle.net/36qhy49g/1/
The init tests all seem to be complete there though, so we can happily play about with code relating to the initialization without hopefully risking damaging anything else.

The addEvents() function is not to your liking.

  function addEvents(handlers = {}) {
    eventHandlers.afterPlayerReady = handlers.afterPlayerReady;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

You want to use just a single handler instead, so we assign a handler variable inside of the code.

  function addEvents(handlers = {}) {
    const handler = handlers.afterPlayerReady;
    eventHandlers.afterPlayerReady = handlers.afterPlayerReady;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

The tests still pass.

We can then update the eventHandlers.afterPlayerReady line to use that handler variable.

  function addEvents(handlers = {}) {
    const handler = handlers.afterPlayerReady;
    // eventHandlers.afterPlayerReady = handlers.afterPlayerReady;
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

The tests still pass.

That handler variable now isn’t really needed inside of the function. It’s not appropriate yet to just replace the existing function parameter yet, because that can break all sorts of other things, so we’ll just perch the new handler parameter on the end of the function parameters.

  // function addEvents(handlers = {}) {
  function addEvents(handlers = {}, handler) {
    const handler = handlers.afterPlayerReady;
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

That causes the code to break because the const is attempting to define a variable that already exists. We can remove const from that line, while leaving everything else on that line unchanged.

  function addEvents(handlers = {}, handler) {
    // const handler = handlers.afterPlayerReady;
    handler = handlers.afterPlayerReady;
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

And the tests still pass.

The next stage is to get something useful from that handler parameter. That means passing in a second function parameter to everywhere that addEvents is being called.

Fortunately, that happens only in one place. We can get a handler from that initEventHandlers function:

  function init(initEventHandlers) {
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    addEvents(initEventHandlers);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

And before we can even pass that handler into addEvents, that breaks one of the tests.

videoPlayer tests > init > loads iframe script
TypeError: Cannot read properties of undefined (reading 'afterPlayerReady')

We have a test that calls the init() method with no function variable parameters, so there is no object in which afterPlayerReady can be found.

Is it appropriate to call the init() method with no function parameters? Yes it can be indeed, so we need a way to get a potential handler without breaking the code. One of those ways is to have the init() method parameter default to being an object, but then we are going to be setting up an event with an undefined handler, which isn’t any good. Another way is to check if the initEventHandlers variable exists, before doing anything with potential handlers in there.

  function init(initEventHandlers) {
    if (initEventHandlers) {
      const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
      addEvents(initEventHandlers);
    }
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

The tests go back to passing, and we can continue.

While making the update here, I also notice that it makes better sense to setup the handlers after loading the iframe script and setting up the apiready parts.

  function init(initEventHandlers) {
    // if (initEventHandlers) {
    //   const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    //   addEvents(initEventHandlers);
    // }
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
    if (initEventHandlers) {
      const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
      addEvents(initEventHandlers);
    }
  }

With this new ordering of things, we can instead check if there are no event handlers to init and return out of the function, before the handler gets done.

  function init(initEventHandlers) {
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
    if (!initEventHandlers) {
      return;
    }
    // if (initEventHandlers) {
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    addEvents(initEventHandlers);
    // }
  }

The tests still pass.

We can now send that afterPlayerReadyHandler variable to the addEvents function.

  function init(initEventHandlers) {
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
    if (!initEventHandlers) {
      return;
    }
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    // addEvents(initEventHandlers);
    addEvents(initEventHandlers, afterPlayerReadyHandler);
  }

Now that all things that use the addEvents() function have both function parameters, we can switch around the order of those function parameters so that the handler comes first.

  // function addEvents(handlers = {}, handler) {
  function addEvents(handler, handlers = {}) {
    handler = handlers.afterPlayerReady;
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }
...
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    // addEvents(initEventHandlers, afterPlayerReadyHandler);
    addEvents(afterPlayerReadyHandler, initEventHandlers);

The tests still pass, so we can now update addEvents() so that it doesn’t use the handlers function parameter.

  function addEvents(handler, handlers = {}) {
    // handler = handlers.afterPlayerReady;
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

The tests still pass.

The handlers function parameter is no longer being used, so that can now be removed.

  // function addEvents(handler, handlers = {}) {
  function addEvents(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

The tests still pass.

This is when I notice that the addEvents() function is now poorly named, because it only deals with the afterPlayerReady event, but doesn’t yet do anything to inform us about that in the function name. We will get to that after removing the second function parameter.

The second function parameter can now be removed from everywhere else that it was used.

    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    // addEvents(afterPlayerReadyHandler, initEventHandlers);
    addEvents(afterPlayerReadyHandler);

The tests still pass.

This is now a good time to rename the addEvents function, to something more suitable such as initAfterPlayerReady.

  // function addEvents(handler) {
  function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }
...
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    // initHandlers(afterPlayerReadyHandler);
    initAfterPlayerReady(afterPlayerReadyHandler);

And the tests still pass.

That also looks to be all of the work that needs to be done in that section to achieve the desired outcome, of having the function be singular in nature instead of plural.

Hopefully this helps to demonstrate the usefulness and benefits of having those tests in place, as well as that it’s not normally as easy as making a simple change.

1 Like

Instead of this: https://jsfiddle.net/x0ugw8q6/

  function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }
  
  function init(options) {
    initAfterPlayerReady(options.afterPlayerReady);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

It would be written this way: https://jsfiddle.net/unc3h0dz/1/

  function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }
  
  function init(initEventHandlers) {
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    initAfterPlayerReady(afterPlayerReadyHandler);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

This would be Singular: https://jsfiddle.net/unc3h0dz/1/

  function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }
  
  function init(initEventHandlers) {
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    initAfterPlayerReady(afterPlayerReadyHandler);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

This would be Plural: https://jsfiddle.net/utb2154f/

 function addEvents(handlers) {
    eventHandlers.afterPlayerReady = handlers.afterPlayerReady;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

  function init(initEventHandlers) {
    addEvents(initEventHandlers);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

It’s not a matter of copy/pasting it one way or another. The above process that was used to work through the problem should always be done instead.

The code in your post is flawed, for reasons that I have already covered in my previous post.

Is there anything from my previous post that you had trouble understanding? Because right now it seems that you either didn’t read it, or didn’t understand what was there.

I just read everything you wrote, but you say how I have it written is flawed.

How is it flawed? In what way?

If that way is wrong how I have it, then there is a right way to do it.

Maybe I am not understanding something.

The name options, should be changed to initEventHandlers

Like this? https://jsfiddle.net/51b2sp68/1/

function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }
  
  function init(initEventHandlers) {
    initAfterPlayerReady(initEventHandlers.afterPlayerReady);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

It is flawed because it breaks the tests.

I recommend that you work through what I did, step by step, from the link that I provided at the start of the post, so that you can gain a clearer understanding not only of the process, but also what you may have been misunderstanding.

1 Like

I just followed your instructions here:

Test passes: https://jsfiddle.net/exj1Lt4o/

Works in the code: https://jsfiddle.net/vuL8ad2c/

Does this get improved better to something else?

  function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }
  
  function init(initEventHandlers) {
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
    if (!initEventHandlers) {
      return;
    }
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    initAfterPlayerReady(afterPlayerReadyHandler);
  }

Does that make this one flawed also?

This one fails in the tests: https://jsfiddle.net/wpuqy0k9/

But the code itself works: https://jsfiddle.net/Lpxfdh9z/

What does that mean?

function addEvents(handlers) {
    eventHandlers.afterPlayerReady = handlers.afterPlayerReady;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

  function init(initEventHandlers) {
    addEvents(initEventHandlers);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

yes that can get improved. Previously the addEvents() function had a default parameter on there. That cannot be done with the initAfterPlayerReady() function because the problem happens before that function is called. We can use that default parameter solution on the init() function parameter instead, letting us remove the if statement.

What does that mean I do?

  function init(initEventHandlers) {
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
    if (!initEventHandlers) {
      return;
    }
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    initAfterPlayerReady(afterPlayerReadyHandler);
  }

I delete the if statement, then what?

We can use that default parameter solution on the init() function parameter instead,

How do I do that? https://jsfiddle.net/b0tn4326/

I’m stuck.

I don’t understand how to do it, even after re-reading your test post.

I don’t understand what that means.

use that default parameter solution on the init() function parameter instead

  function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

  function init(initEventHandlers) {
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    initAfterPlayerReady(afterPlayerReadyHandler);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

You would have the default parameter being used on the original function. Here it is.

  function addEvents(handlers = {}) {
    eventHandlers.afterPlayerReady = handlers.afterPlayerReady;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

It’s still failing, I’m confused still. https://jsfiddle.net/xs8ob25h/

Did I forget to do something?

  function initAfterPlayerReady(handler = {}) {
    eventHandlers.afterPlayerReady = handler.afterPlayerReady;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

  function init(initEventHandlers) {
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    initAfterPlayerReady(afterPlayerReadyHandler);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

Well I have recently said the following, which seems to have been ignored.

I’m pretty sure that if you do that from the code at https://jsfiddle.net/b0tn4326/, that things will come right.

1 Like

Singular code is now fixed:

Passes in tests: https://jsfiddle.net/8q546gvm/

function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
  }

  function init(initEventHandlers = {}) {
    const afterPlayerReadyHandler = initEventHandlers.afterPlayerReady;
    initAfterPlayerReady(afterPlayerReadyHandler);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
  }

Improving upon that, next, I can do this?

Passes in tests: https://jsfiddle.net/og8u569r/1/

Works in code: https://jsfiddle.net/0so5L1uc/2/

function initAfterPlayerReady(handler) {
    eventHandlers.afterPlayerReady = handler;
    events.afterPlayerReady = new Event("afterPlayerReady");
}

function init(initEventHandlers = {}) {
    initAfterPlayerReady(initEventHandlers.afterPlayerReady);
    loadIframeScript();
    window.onYouTubeIframeAPIReady = onYouTubeIframeAPIReady;
}

Yes you can.

So far it has been just refactoring that has being done to those functions.

The next potential improvement is where the init function doesn’t have an object as its function parameter, and instead just has the afterPlayerReady handler as its function parameter.

That is a more serious change though, as it changes the interface of the videoPlayer method. It is a redesign, instead of a refactor.

  • A refactor is where the internals of videoPlayer change, but the inputs and outputs remain the same.
  • A redesign is where the inputs and/or outputs of videoPlayer are changed.

To redesign the code, the design document needs to first be changed. That design document is the tests. You would update the few tests that deal with the init method, so that only the handler is given as the function argument. That should cause the tests to break because you have different requirements of the videoPlayer code, resulting in you updating the init function in the videoPlayer code so that the tests go back to passing.

1 Like