Refactor overview principles0:00
Today, I'm working on the Laravel Mixed Code Base, and I'd like you to come along for a particular refactor. So this is written in JavaScript, but a quick note, if your instinct is, well, I know PHP, I wish you would show me a refactor in PHP instead, try to get out of that mindset, because the wonderful thing about refactoring is the basic principles and techniques are true regardless of the language. So the way I would approach this in a JavaScript file is almost identical to how I would think in a PHP file. Okay, so let's take a look at this class. Now it's called Verify.
Review Verify responsibilities0:33
Okay, so let's take a look at this class. Now it's called Verify. Immediately, I don't love that name, but we would trigger it like this. So you might say verify.js, and then you would give us an entry file and an output path. So with Mix, you can see very quickly here, wouldn't you call something like mix.js? You want JavaScript compilation. Well, at some point, this method will be called, and it handles basic assertions. It's a way for us to quickly detect, did you use the API correctly? So we can see an example of checking for JS or your basic preprocessor, like mix.ls or sass, combining files, checking for if a file exists, once again, all basic assertions.
So for every dependency in your list, it's going to try to resolve it. And if it could find that, well, that means it's available. We're good to go. So we will reject that from the array. We'll remove it. So let's say that one's available. Now the array looks like this. So at that point, we tap the dependencies, and we call this method or function installDependencies. If we take a look at that, well, that's logging something to the console.
We are logging things to the console, so we're providing user feedback, and we are then building up install commands and triggering them. This is a good example of what I would say too many responsibilities. In general, I'm not a big fan of getting on the roof and yelling single responsibility principle, and then everything is supposed to be better as a result. However, I think the basic concept is a good one. In this case, we can see, well, the basic responsibility of this class is to perform basic assertions for the API. But now I'm seeing all this code related to installing dependencies, and that feels like a very separate responsibility.
Rename Verify to Assert3:01
But now I'm seeing all this code related to installing dependencies, and that feels like a very separate responsibility. So that's a good hint to me that maybe you should refactor this. So I'd like you to come along as we do that. We're going to do this in two steps. First, I said I didn't like this class name. What would be better? Well, how about Assert? It's a little more clear, isn't it? So let's do this.
It's a little more clear, isn't it? So let's do this. In my source file, I'm going to do a find in folder. We'll look for verify and replace it with assert, and we'll check the git commit, but I'm going to trust a full search and replace without checking first. And we'll save all. Next, let's go to my test folder, and let's look for verify real quick. Okay, we have one use case, so I will update that to assert. And then finally, this file name can be renamed to assert. Okay, so let's close everything and run my tests.
And then finally, this file name can be renamed to assert. Okay, so let's close everything and run my tests. Okay, so if I scroll up, you can see all the tests are passing. Let's go ahead and commit this. Change verify class name to assert. Okay, great. So now we can move on to step two. Create a scratch file here. So before, we had verify.tendencies, and once again, you would give us an array of npm packages that we must verify are available, and if they're not, we install them.
Extract Dependencies installer4:21
So before, we had verify.tendencies, and once again, you would give us an array of npm packages that we must verify are available, and if they're not, we install them. We want to change this, so we want to extract this. So you'll remember, if we switch back, we had this function called installDependencies. I'm going to bring that over. Often when you have two words that form a method name, this is often, not always, but often a sign that maybe you're missing an extraction. So in this case, a technique I often use is the verb, the action at the beginning will be the method name. What comes after the action should be the class name.
be the method name. What comes after the action should be the class name. So that would give us something like this or something like that. Okay, so why don't we strive for this public API? To begin, I'll create a test. I'm using a test framework called Ava, so we'll pull that in. We'll set up our first test here, and what are we testing? Well, how about this? It installs a single dependency. It's basically what I want to check here for.
It installs a single dependency. It's basically what I want to check here for. So now let's go to our scratch file. This is the API, the public interface that we want to use, right? Now where do I pass the dependencies? Should it be here or through the constructor? I think either are fine, but let's do the constructor. So if you had something like this, ultimately, well, how do we verify this? Because I don't actually want to trigger npm install browser-sync every time I run this test.
That's what we want to do here. Now the way we can trigger this, I'll show you. We can import a process from child_process. This is available everywhere with Node. You can do things like this. You could say process.exec, or in our case, execSync. Same thing, but it does it synchronously. So for example, if you ran this code, that would trigger your install command. So it sounds like we need to stub this out because I don't actually want to do it. I just want to say, well, I need to make sure I called this method with this command.
So it sounds like we need to stub this out because I don't actually want to do it. I just want to say, well, I need to make sure I called this method with this command. Okay. So let's stub it out. Now I use a tool called signIn for this, so it works great. Here we could say signIn, stub, process, and specifically execSync. So I don't actually want to trigger that functionality. I'm going to stub it out. So now our final test could be, well, I expect process.execSync should be called with this command here.
So now our final test could be, well, I expect process.execSync should be called with this command here. So let's grab that. That is our test. All right. Let's get started. Let's run our test. Ava test dependencies, and immediately it blows up. So attempted to wrap an undefined property execSync as a function. Let's see.
So attempted to wrap an undefined property execSync as a function. Let's see. Oh yeah, sorry. That should not be in quotes. One more time. Okay. So dependencies is not defined. Of course not. We're trying to instantiate a class that doesn't exist. Let's import it.
We're trying to instantiate a class that doesn't exist. Let's import it. And then create the file. Class dependencies, and then I will export that. Okay. Let's do it again. Now the install method does not exist. Of course not. Run it again. Okay.
Run it again. Okay. So now we get to a good failure. So we expected process.execSync to be triggered with this command, but of course we're not doing anything at the moment. Okay. So now I can begin migrating over some of the code that we had here. So we have our install command. Let's come up. Here's what we had.
Let's come up. Here's what we had. So I'm going to take all of this code and start migrating it over. I'll paste that in. So install, well, you'll remember in our test, we're passing through the dependencies to install through the constructor. So let's accept that. Like so. Now, rather than list, I would say this.dependencies.
Now, rather than list, I would say this.dependencies. Okay. Next. So we filter it down. We reject any dependency that is already available. And then of the ones that remain, we will tap that array and then call this installDependencies function. Yeah, that's the other thing I want to get out. So let's comment that out. We're no longer using it.
So let's comment that out. We're no longer using it. And here's this new function. So I'm going to grab all of this and move it over. Like so. And reformat. Okay. So let's see. We logged to the console. In this section, you can see we're basically building up a command string, or we're building
We logged to the console. In this section, you can see we're basically building up a command string, or we're building up the command that we will ultimately execute. So execute. Let's pull that in. Let process equals require child_process, just like we did before. So then we'll scroll down and we can say process.execSync, the command. Next we have if abortOnComplete. So yeah, we need to migrate that over. So it sounds like we should accept that here.
So yeah, we need to migrate that over. So it sounds like we should accept that here. Mostly you don't need to know about this. In certain situations, after the dependencies have been installed, we need to force a rerun of npm run dev, essentially. And in other cases, you don't. So by default, we won't require it. But if you need to, you can see, well, we'll check for that and we'll call this abort function. That too is on assert. So let's grab that, get rid of that entirely.
It's trying to call reject on an array. And by default in JavaScript, that's not available. It's something that I add myself. So you'll see right here, we add to the array.prototype. So a tap function and a reject function. So when I load up Nix, this is included. But in our case, we're just testing a single file. So why don't we just pull it in, like so, and give it another run. Okay, next. So we're checking to see if a file exists.
Okay, next. So we're checking to see if a file exists. But once again, we have an imported file. All right, let file equals require our File class and run it again. Okay, so now it's failing. Yeah, same thing again. We're not triggering this the way we expect. Let's see why. Right down here. Yeah, I want to figure out what was the command that was built up.
So here's what I'm going to do. I'm going to delete this temporarily, but we could also do it in reverse. We're going to assume npm by default, and then we will fake a yarn file. So if I give this another run, I bet we get green. And we do. Great. Okay, so let's do this. Let's write another test, maybe two, and then I can refactor that main code. It installs multiple dependencies. So now let's say we need browser-sync, but also browser-sync or whatever it happens to.
It installs multiple dependencies. So now let's say we need browser-sync, but also browser-sync or whatever it happens to be, webpack-plugin, something like that. Now, if we call install, ultimately, the install command should be this. Okay, let's give that a run. Hopefully, it's still at green. It's not. Attempted to wrap execSync, which is already wrapped. Okay, so up here, we wrapped it. And then down here, we tried to wrap it again.
this code no matter what. And what I want to do is restore the original behavior. So process.execSync.restore is how we do that. Okay, let's give that another run. It passes. Great. So what about this other section, yarn? All right, so it sounds like I need a way to say, well, assuming that we do have a yarn.lock the user probably wants to use yarn to pull in the dependency. Okay, let's create another test.
the user probably wants to use yarn to pull in the dependency. Okay, let's create another test. Actually, I'll grab this guy here. It installs a single dependency with yarn. So what we'll do here, let's pull in file. We're going to stub that out. Right down here, let's say signin.stub file, and specifically the exists method. So right here, whoops, we know that we're going to check if the file exists. So let's just say, all right, stub it out, and we're going to force a return value of true so that we can go down this particular path.
So let's just say, all right, stub it out, and we're going to force a return value of true so that we can go down this particular path. Okay. Now, when we run this code, it should actually build up yarn.add, browserSync. All right, let's give it a run. They all pass. Great. And now here, you see the console.logs there? You can even remove that. Maybe before each test, we'll empty it out.
Refactor installer internals14:42
You can even remove that. Maybe before each test, we'll empty it out. Great. So now I can do a little bit of refactoring. Because right now, yep, that tap method, a little too much for my taste. So let's see what's going on here. The first thing I see is this. We are building up the command that should be run. So what if instead I said, let command equals this buildInstallCommand? That's what we're doing.
So what if instead I said, let command equals this build install command? That's what we're doing. All right, fair enough. build install command based on the array of dependencies, and I'll paste that in. Based on the array of dependencies, and I'll paste that in. Okay, rerun the test. Are we still at green? No. So even that little refactor broke everything. Oh yeah, of course, because we forgot to return.
So even that little refactor broke everything. Oh yeah, of course, because we forgot to return. All right, run it again. It still fails. So let's see. Oh yeah, we forgot to pass the dependencies. All right, one more time, and I hope we get green, and we do. All right. Can we clean this up further? Well, we have a command that we want to return, right?
Can we clean this up further? Well, we have a command that we want to return, right? But then we check, well, if you're using yarn, we're going to change the command. So why don't we just do that check first? If you're using yarn, here's the command to return. Otherwise, return this one. A little bit cleaner. It makes a difference. So if we give that a run, we still get green. Great.
So if we give that a run, we still get green. Great. Next, right here. So we're assuming dependencies could be a single string, or it could be an array of dependencies. So we check for that. Another way you can do it is to just force an array no matter what, so that you never have to perform a conditional. You could always say array.concat the dependencies. And what's nice about concat is you can give it a string here or an array, and it will just merge or concatenate all of that together.
And what's nice about concat is you can give it a string here or an array, and it will just merge or concatenate all of that together. So what we could do is force an array and then do something like this. So these two are compatible. I like that one more. We run it, and we still get green. Okay. Next, let's scroll back up. Here I can see I created a variable, but I only use it once. So in those situations, I will often inline it like so.
Here I can see I created a variable, but I only use it once. So in those situations, I will often inline it like so. Give it another run. Oh, we made a mistake. Okay, missing a closing ). All right. Back to green. All right, next. So what's going on here? We're building an install command.
So what's going on here? We're building an install command. We log an initial message to the user, and then if we need to abort on complete, we log another message for the user, and then we call process.exit. Actually, real quick, message should be called that. Anyways, I think we can group some of this together to clean things up. So maybe we'll have our own execute method here. So let's give this a shot. We'll have execute a command. And what I'll do is pass this through.
We'll have execute a command. And what I'll do is pass this through. All right. So ultimately, we will defer to process.execSync, like so. But now also, because we are executing a command, I think it's safe to move that console.log right down here. It's connected. Further, if we want, we could include the abortOnComplete as well, since it's connected to executing the command. So why don't we pass that, like so.
to executing the command. So why don't we pass that, like so. And let's see, abort on complete, and paste it in. Okay, I'll let you take a look at that. So now we have a single method for executing the command. We send the $user a message, and then mostly we execute it, and then, optionally, exit the process. So if we come back, here's our new tap method. Let's run the tests. Still at green, so we're looking good.
Let's run the tests. Still at green, so we're looking good. Let's take a look. All right. So our constructor is good. Install. We're just filtering down this collection here. No problem. We have a method that wraps how we execute the command. Now, some people would even say, well, this should be in its own class as well.
We have a method that wraps how we execute the command. Now, some people would even say, well, this should be in its own class as well. That's up to you. That's where I think single responsibility principle can be taken as far as you possibly want it. You could even say it's not the responsibility of this class to log something to the console, and you now need a ConsoleLogger class. You can go down that rabbit hole as much as you want. I would instead recommend go down as far as you need to to feel comfortable, what feels good to you.
I would instead recommend go down as far as you need to to feel comfortable, what feels good to you. In this case, I think it's fine. Anyways, if we scroll down and then we have our method for building up. Yeah, once again, this could be install command constructor. That could be the name of the class. It just doesn't matter. This is perfectly fine to have it as a method. Okay. So now think about it.
This is just a simple explanation with no code.
Preserve API during refactor20:25
So you don't always have the luxury of just deleting code whenever you want. That's where you have to ask yourself, do others depend upon this code or this method being available? And if the answer is yes, you either need to force a breaking change or, better, what you could do is something like this. Keep the method as it is, but now you can defer to your new class. So you could do something like this. That's a good stopgap. You maintain the current API, so you're not forcing a breaking change. But the underlying code can now defer to your new class.
You maintain the current API, so you're not forcing a breaking change. But the underlying code can now defer to your new class. And the same would be true here. In this case, that was a double, so I can remove that. All right, so all of this code, doesn't it feel good? We're just getting rid of all of this junk. And if we scroll back, we can see that now, and actually in my case, this was for an example, in my case, I'm the only one who would ever trigger that code. So I can reliably remove that. Anyways, now take a look at this class.
So I can reliably remove that. Anyways, now take a look at this class. It does exactly what we thought it should do at the very beginning. It performs basic assertions on variables. And I can get rid of that as well as that. Yeah, very, very simple. And that's how I know it's a good refactor. Suddenly, once we remove all of that other code, this file seems a lot more cohesive. And further, it's more clear. So now when a user thinks, OK, well, I need to change something related to when dependencies
