Quite the opposite. config.player is being changed. Normally we would return that kind of change at the end of the function, but there’s nothing that we control that can receive that, so instead it has to be changed inside of the function. As a result, that change occurs at the end of the function.
Yes, we should do that.
Next would be doing this? https://jsfiddle.net/6by9kxcq/1/
Is this one good?
If this is good, what is next to be done?
function getIframe() {
const player = config.player;
return Object.values(player).find(
(item) => item.nodeName === "IFRAME"
);
}
The shufflePlaylist function is next to be done. The config.player is repeated all throughout that function, so define a player variable at the top of the function and use that player variable throughout the function instead.
I did that here: https://jsfiddle.net/cnvuhtg2/
The next refactoring involved tidying up the following code:
function checksVideo(video) {
const hasVideo = video && video.classList.contains("video");
if (!hasVideo) {
throw new TypeError("Element needs a video classname.");
}
}
function addPlayer(video, Ids) {
checksVideo(video);
const videoId = video.dataset.id;
if (!videoId && !Ids.length) {
throw new TypeError("A video id is needed.");
}
const options = createOptions(Ids || videoId);
I want to pass video and the videoIds to a separate function that checks that things are okay, and returns something useful from that function.
As a result, we now have a separate checkVideoIds() function where the video and the videoIds are checked and warned about if need be.
function getVideoId(video, videoIds) {
const hasVideo = video && video.classList.contains("video");
if (!hasVideo) {
throw new TypeError("Element needs a video classname.");
}
const videoId = video.dataset.id;
if (!videoId && !videoIds.length) {
throw new TypeError("A video id is needed.");
}
return videoId;
}
function addPlayer(video, videoIds) {
const videoId = getVideoId(video, videoIds);
const options = createOptions(videoIds || videoId);
That results in much cleaner addPlayer() code, and because I was slightly unsettled by the checkVideoIds() function doing more than the name suggests, it’s been renamed to be getVideoId() instead.
I did that here: https://jsfiddle.net/afjkz9m1/1/
Are we almost up to doing this?
addPlayer() function to have just a single videoIds parameter, so that regardless of it being undefined, a single video id, an empty array, or a full array of video ids, it will be capable of properly handing everything.
You might notice that I’m working my way down vertically from the top of the code. When I get to the bottom with no more refactorings (which is restructuring the code without needing to change any tests (which would be a redesign instead), we’ll be done with this lot of refactoring.
The next thing to refactor is the addPlayer() function.
Here’s how it’s called:
function onYouTubeIframeAPIReady() {
...
config.player = addPlayer(frameContainer, videoIds);
}
and here’s what the addPlayer() function does.
function addPlayer(video, videoIds) {
const videoId = getVideoId(video, videoIds);
const options = createOptions(videoIds || videoId);
config.player = new YT.Player(video, options);
const iframe = getIframe(config.player);
const eventHandler = config.eventHandlers.afterPlayerReady;
iframe.addEventListener("afterPlayerReady", eventHandler);
return config.player;
}
You’ll notice that the addPlayer() function does things with config.player, but that is also being assigned by the code that called the addPlayer() function. That means that there is no need to use config.player in the addPlayer() function at all.
We can’t improve that yet because the getIframe() function still relies on a global access to config.player. Note: globals are bad and to be avoided if possible.
So, improving the getIframe() function is what next needs to be done, after which improving the addPlayer() function can then occur.
The call to the getIframe() function is called with an argument. That is all good.
const iframe = getIframe(config.player);
The getIframe() function itself though doesn’t do anything with that argument. Instead the function ignores it by having zero function parameters, and instead reaches out to the config.player global once again.
function getIframe() {
const player = config.player;
return Object.values(player).find(
(item) => item.nodeName === "IFRAME"
);
}
We need to update the getIframe() function so that it uses a function parameter called player, letting us remove that top line in the function.
Like this: https://jsfiddle.net/Lraxgejq/1/
function getIframe(player) {
return Object.values(player).find(
(item) => item.nodeName === "IFRAME"
);
}
Do you want me to do that here also? https://jsfiddle.net/Lraxgejq/3/
function shufflePlaylist(player) {
player.setShuffle(true);
player.playVideoAt(0);
player.stopVideo();
}
Yes, that’s good too.
Now that getIframe() is doing its thing based only on the parameters given to is (which is how all good functions should behave), we can update the addPlayer() function too, to replace config.player with just player, and use const player for the assignment.
There’s a lot of big name contents going on here such as idempotence, but basically the ideal is for a function to use parameters for inputs and returns for outputs, resulting in more reliable code.
Next after that is to do some refactoring of the manageCover test, by moving functions above the beforeEach() and afterEach() functions down below them instead.
we can update the addPlayer() function too, to replace config.player with just player, and use
const playerfor the assignment.
That I did here: https://jsfiddle.net/ng2z8wq6/
Next after that is to do some refactoring of the manageCover test, by moving functions above the beforeEach() and afterEach() functions down below them instead.
I did that here: https://jsfiddle.net/mku6419c/
The commented chart that was placed in the “has no playlist” test. That chart relates to the test that above as well, so can you move that up above the describe line so that it’s between the “has a playlist” and “has no playlist” tests.
That’s about all for he refactoring, and we can move on putting together.a suitably failing test.
In a change that was made to the code before the refactoring, loop was removed due to single videos not needing it, but we should still have loop when there are multiple videos. We can add that requirement to one of the tests.
With the “Has a playlist” test, add an expectation to the end of the test that expects playerVars.loop to be a value of 1.
I did all that here: https://jsfiddle.net/fegkL73o/
If that is good, next we need a passing test.
Passing test: https://jsfiddle.net/u3bhgm61/3/
Is this good?
I got it to pass, but it doesn’t mean I did it right.
or, maybe we can refactor from here?
I added this:
function setLooping(options) {
options.playerVars.loop = 1;
}
Then I added: setLooping(options);
to:
if (isPlaylist(videoIds)) {
setLooping(options);
options.playerVars.playlist = createPlaylist(videoIds);
} else {
options.videoId = videoIds;
}
return options;
}
And this is how it looks:
function setLooping(options) {
options.playerVars.loop = 1;
}
function createOptions(videoIds) {
const options = {
height: 360,
host: "https://www.youtube-nocookie.com",
width: 640
};
options.playerVars = {
autoplay: 0,
cc_load_policy: 0,
controls: 1,
disablekb: 1,
fs: 0,
iv_load_policy: 3,
rel: 0
};
options.events = {
"onReady": onPlayerReady
};
if (isPlaylist(videoIds)) {
setLooping(options);
options.playerVars.playlist = createPlaylist(videoIds);
} else {
options.videoId = videoIds;
}
return options;
}
Refactoring, maybe this is all that is needed to be done?
That means removing this:
function setLooping(options) {
options.playerVars.loop = 1;
}
And only use this: options.playerVars.loop = 1;
https://jsfiddle.net/u3bhgm61/6/
if (isPlaylist(videoIds)) {
options.playerVars.loop = 1;
options.playerVars.playlist = createPlaylist(videoIds);
} else {
options.videoId = videoIds;
}
return options;
}
Well done, that’s a good use of the process, and the loop now works appropriately regardless of whether it’s a single video or multiple videos.being played.
There is a little bit more refactoring to do in the test.
The information chart that we’re using shows videoId, loop, playlist, and shuffle in a particular order, but in the test we’re doing playlist first and then loop, which are in a different order.
When things are in a different order from each other that’s usually because there is a good and special reason for why they must be in that different order. If there is no good and special reason and they are still in a different order, we tend to waste time and attention in trying to figure out why they are in that different order.
Let’s make things easier for us moving forward, by ensuring that those things remain in the same order. That means moving the loop expectation above the playlist expectation.
That also gives us the next test that we should be doing, which where we add another expectation to that test. A playlist should not have videoId at all, videoId is expected to be undefined.
https://jsfiddle.net/fgk2b7Lr/1/
//then
const playerVars = options.playerVars;
const playlist = "0dgNc5S8cLI,mnfmQe8Mv1g,CHahce95B1g,2VwsvrPFr9w"
expect(playerVars.loop).toBe(1);
expect(playerVars.playlist).toBe(playlist);
});
That also gives us the next test that we should be doing, which where we add another expectation to that test. A playlist should not have videoId at all, videoId is expected to be undefined.
That means, making a new test?
https://jsfiddle.net/7w1k5h8o/2/
Is it supposed to fail?
it("A playlist should not have videoId at all", function() {
//given
options = undefined;
//when
initVideoPlayer([
"0dgNc5S8cLI",
"mnfmQe8Mv1g",
"CHahce95B1g",
"2VwsvrPFr9w"
]);
//then
const playerVars = options.playerVars;
const playlist = "0dgNc5S8cLI,mnfmQe8Mv1g,CHahce95B1g,2VwsvrPFr9w"
expect(playerVars.loop).toBe(1);
expect(playerVars.playlist).toBe(playlist);
expect(playerVars.videoId).toBe(undefined);
});
Each of the four things that we are testing in regard to it being a playlist, all have exactly the same initial conditions and setup, so we can keep the expectations in the same test.
The chart that we’re using has four different things in regard to the same playlist, so the “has a playlist” test can have four expectations in it.
In https://jsfiddle.net/fgk2b7Lr/1/ code, please add another expectation to the “has a playlist” test that options.videoId is undefined. It’s options.videoId that gets checked because the API doesn’t have videoId in the playerVars object. Instead the API has videoId in the options object for single videos, so it shouldn’t be there when a playlist is being used.
Please don’t make a separate test that mostly copies another test, as that is only needlessly duplicating what shouldn’t be duplicated. Yes, we could remove that duplication if we notice it when refactoring, but don’t make extra work for us that’s not needed.
https://jsfiddle.net/03foq8dv/
it("has a playlist", function() {
//given
options = undefined;
//when
initVideoPlayer([
"0dgNc5S8cLI",
"mnfmQe8Mv1g",
"CHahce95B1g",
"2VwsvrPFr9w"
]);
//then
const playerVars = options.playerVars;
const playlist = "0dgNc5S8cLI,mnfmQe8Mv1g,CHahce95B1g,2VwsvrPFr9w"
expect(playerVars.loop).toBe(1);
expect(playerVars.playlist).toBe(playlist);
expect(playerVars.videoId).toBe(undefined);
});
Move that videoId line so that it’s above the loop line, for the same reason as before to keep it in the same relative order as the chart.
No failing test occurs because the code already does what that expectation is checking. That means that some of our earlier coding work in regard to videoId with a single video did more than it was supposed to do because it also affected how things work with multiple videos. Oops. Fortunately, because the test passes that earlier work did the right thing, but that is an issue that we can learn from. Try to only affect things as minimally as possible to make the tests pass.
That test passes already so it acts as a confirmation that things are working well in that regard.
Refactoring still gets checked, and there doesn’t seem to be any refactoring to do because we didn’t change much.
However, I think that you’re right from earlier that these expectations should be in separate tests instead of being combined together into one test. Copy that “has a playlist” test out to two other tests. There are three expectations so we’ll have three different descriptions, each with one expectation at the end of that test:
“has no videoId when using a playlist” → expect(playerVars.videoId).toBe(undefined);
“loops a playlist” → expect(playerVars.loop).toBe(1);
“has a playlist” → expect(playerVars.playlist).toBe(playlist);
I did that here: https://jsfiddle.net/to3fhzLm/
Can you let me know when I can, or when it is a good time to update the video player code.
We’ve made lots of changes and updates and so it hasn’t been updated in some time.
As I know, some of the stuff in the test code does not get added to the video player code, so some of that can be tricky.
It’s not a simple copy and paste.
I can try it when you say it is a good time and we can fix things from there, unless if I was able to be able to do it on my own.
When we finish with the existing chart of single/multiple video behaviour, that would be a good time.
The overall goal with this next bit of refactoring is to have a consistent set of tests both with single videos and with multiple videos.
The single videos are easy to do, for that just means renaming the describe section from “has no playlist” to “with a single video”
Just above that add a describe section called “with multiple videos” and move inside of there one test for each row that’s on the chart, and have them in the same order that’s on the chart.
I did that here: https://jsfiddle.net/ehaqjout/2/
Did I do this right?
If I understood you correctly.
Just above that add a describe section called “with multiple videos” and move inside of there one test for each row that’s on the chart, and have them in the same order that’s on the chart.
/* single | multiple
-------------------------------------
videoId | in options | none
loop | none | in playerVars
playlist | none | in playerVars
shuffle | none | is shuffled*/
describe("with multiple videos", function() {
it("checking single video for a video id", function() {
//given
video.dataset.id = "0dgNc5S8cLI";
//when
initVideoPlayer();
//then
expect(options.videoId).toBe("0dgNc5S8cLI");
//cleanup
delete video.dataset.id;
});
it("single video doesn’t loop", function() {
//given
options = undefined;
video.dataset.id = "0dgNc5S8cLI";
//when
initVideoPlayer();
//then
const playerVars = options.playerVars;
expect(playerVars.loop).toBeUndefined();
//cleanup
delete video.dataset.id;
});
it("single video doesn’t use playlist", function() {
//given
options = undefined;
video.dataset.id = "0dgNc5S8cLI";
//when
initVideoPlayer();
//then
const playerVars = options.playerVars;
expect(playerVars.playlist).toBeUndefined();
//cleanup
delete video.dataset.id;
});
it("single video doesn’t shuffle", function() {
//given
fakePlayer = undefined;
video.dataset.id = "0dgNc5S8cLI";
initVideoPlayer();
//when
triggerOnReady();
//then
expect(fakePlayer.setShuffle).not.toHaveBeenCalled();
//cleanup
delete video.dataset.id;
});
});
describe("with a single video", function() {
});
Next,
4 tests go in here?
We’re creating 4 new tests?
describe("with a single video", function() {
});