 Buddy, my name is Nahan, and this talk is Refactoring Legacy Code, Guided by Simple Design. So I'm just going to get started. I'm not taking any questions during this presentation. There'll be like two minutes at the end where you can ask questions if you want. And I'll be around for the rest of the conference, or afterwards I'll hang out outside. And you can ask me questions, and you'll see why in a minute. So it just goes fast. The content is about 43 minutes. So here we go. So when people talk about simple design, usually it's in the context of new software that they're writing. And there are four basic rules of simple design. One is that they pass all the tests, second reveals intent, third don't repeat yourself, and four fewer parts. There's also one rule of legacy code refactoring. And that rule of legacy code refactoring is don't break it. So whenever we're dealing with legacy code, we want to make sure that anything that we do is safe. So we're going to have a little bit of code here that we're going to look at. And this is an extract from a big legacy code system that I worked on. This is just four classes that I pulled out of there. And this will kind of give us some context about what we're going to be working on. The first class is this custom job execution listener. And it implements this class that is based on the spring framework. And so there's a lot of spring dependencies and so on in there. There's another class, date time util that has, as you might imagine, date time utilities, lots of little things having to do with timestamps, local dates, all that kind of stuff there. We have a file utility class that, as you might expect again, is also about processing files. Very exciting, very long file. As you can see, the scroll bar going down on the right. It's a pretty big one. And the last and final file is this constants file. As every good project has a global constants file that everybody can share data with each other. That's always a good idea. So pretty much every legacy code base I look at has these types of components. So we're going to start with Rule 4, which is fewer parts. So what does fewer parts mean? In the context of legacy code, one of the things that I look at is this thing about dead code. So there's a lot of code in many legacy code bases that's never actually used. So the first thing that we do here is that we let our compiler help us. So in Eclipse, there's a bunch of warnings that you can enable that will tell you about what things are not actually used. In this case, we're turning on things like when variables are not used, local variables, method parameters, type parameters, unnecessary declarations of exceptions, and so on. So once we turn those on, we can open up the little problems tab in Eclipse. And it will show us the warnings. So at this point, there's 10 warnings that are showing up. And some of these things are things like imports are not used, and so on. So the next step after we find the dead code is to eliminate it. So things like this are super easy, this import array list. So we're trying to do safe refactorings. We don't want to do anything that's going to break it, because that's rule one of legacy code refactoring. So we can get rid of things like imports that aren't being used. We have things like this where it's a private method that is not called by anything inside the class. That's safe to remove. You can just get rid of it. You don't even have to really think about it too much. The only exception to things like this is when you're doing some sort of reflection. But if you know your app doesn't use reflection, then you're golden. If it does use reflection, you need to do a little bit more analysis. So there's a few different ways to do this removal. You can click on the little warning icon in the gutter over here on the left. Or you can click on the warning and hit Control-1 or Command-1, which is Quick Fix in Eclipse. And that will go ahead and just remove the thing. This is an example of a local variable that's not used. There's an int counter equals 0 up at the top, i equals 0. And then it just gets incremented inside this loop. It's not doing anything with that value. So we just go ahead and delete it. Eclipse recognizes that value is not actually used for anything, so we can delete that out. The last one that we have is this unused parameters. This is actually one of my favorite refactorings, getting rid of these unused parameters, because it really ties together in couples code in ways that you don't really want. So this secondary file parameter here is not actually used. You may not be able to see it from the back, but it's all these warnings are underlined in yellow when there's a problem. So you can see right above on line 73, it's passing in an empty string for that secondary file name. So in Eclipse, there's a refactoring. It's change method signature, command-option-c. And when you click on that, it'll give you basically a description of what the method signature is. So in this pop-up box, it tells you the name and the two parameters that are there. You can click on the second parameter, hit Remove. And you can see you'll watch as the parameter gets removed from the calling site, as well as the method signature itself. So you can see that empty string that was in there is no longer there. And oftentimes, when you do things like removing method signature parameters, that effect cascades through other classes. And you start being able to remove more and more things. This might be a little harder to see, but what I'm showing here is just committing the code. So after every little small refactoring you make, a very good idea to start committing. So these are safety factorings. Get them into your source control so you don't have a bunch of things lying around that if you start making mistakes, you want to be able to quickly revert back to what you were doing. All right, so our next step of minimize moving parts is to find more dead code. So there's a tool that I really love to use, because Eclipse only tells you so much. There's a tool called Unused Code Detector. Anybody use that here? You used it before? OK, it's a really great tool for Java, or sorry, not Unused Code, Unnecessary Code Detector. It's kind of more strong than Unused. So you can install that from the Eclipse Marketplace. And you run it by just selecting the UC Detector menu and then just run it on your project. And it'll pop up a whole bunch more warnings for you. So this one found now 149 things. This will tell you all sorts of things like even public methods that are not used by anybody in your application. It'll show you methods that are only used by tests. So that means that at some point they were probably used, but they're no longer used and so on. So we're going to start eliminating some of this stuff. And the point of eliminating all this stuff is so that when you actually go in to change your code and add new features to it, you don't have to spend so much time looking through the stuff that doesn't matter. You want to be able to get in there quickly and get out. So this is an example of some of these file date time patterns. And part of the reason some of these are unused is because I did extract this out of a larger code base. But in reality, a lot of these were unused until I got my hands on it. So again, there's little different ways that you can do this. You can delete them one at a time. Also in this case, there's a whole bunch of them that are the same types of these constants that are not needed, so you can just click on them all at once and just delete them all. All right, so then here is another one. These are methods that are unused. So these, again, are public methods. And these public methods even are fine to delete as long as your application is not an API for something. Obviously, if these are public APIs, then they could be used by external people. So you want to be careful of that. But if you know that this is an internal project that's not being used by anybody else, it's safe to delete. So these are, again, fields that are not being used. We've got more of these. And I'm just doing it in a variety of different ways, just you can right-click on them and click Remove, or you can click on the Quick Fix, and so on. So it's just going to go through here and delete some more. You can see there's 32 warnings left, so down from 100 and whatever it was, 149 or something. The other thing that this UC detector tells you is that you can change visibility of things. So it'll try and offer that you can change something from public to package level, and so on. I generally tend to ignore those warnings. They're not as useful, I think. So just reformatting the source, since it deleted those lines of code, but left a bunch of white space. So clean that up. So then we clean it off, and we see that after deleting some of those methods, there are now some leftover things that Eclipse found that were no longer needed. So cleaning up those things as well. Do another commit. Can you guys see this screen from the back? Probably not too good. Anyway, it's just commit and putting in a little message. All right, so the next one is we're going to run UC detector again, because once you delete a bunch of methods, there are other methods that we're calling those methods that are now no longer needed. So you kind of keep going through this process repeatedly until you get to the point where there's no new things popping up. So here in this case, there's a few more things that are saying you can delete. You've got to notice that sometimes the suggestion that Eclipse gives you are things like ignore this warning. Well, in general, we don't want to ever ignore warnings. We want to take care of the warnings and fix them. They're warnings for a reason. They're not there to be ignored. So again, we'll move some imports. This should go pretty quickly. Delete some more stuff. 21 to go. So these classes are already significantly smaller. So we're going to run it one more time. 22 things remaining, and there it goes. The nice thing about doing this on recorded is that I can speed up the parts that are super boring. So there we go. So now I don't actually type that fast. I think this is the last couple. This is where I accidentally was overzealous and deleted something that shouldn't have been deleted, so I put that back in. Last one. Got rid of that. And now we think we're going to commit. So we'll commit that change. Again, committing is very important when you're making these types of changes so that when we do make mistakes, because inevitably, as careful as we're being, sometimes when you're doing this deletion stuff, you kind of get into the zone and just kind of start doing it. And then you might realize that, oopsie, I did a little bit too much. And so it's important to be able to go back to where you were without having to start completely from the beginning. So our next step of minimize moving parts is removing unused classes. So we have this class here, this constants interface, actually. And we can look and see where these things are actually used. So there's a constant called blank. It's used in exactly one class. And this default input file path is used in also one class. Same with these other two. They're both used in all of these are used in one class. The split regex is used in the file util class. Unknown is used also in the file util class. So these things are global constants, but they're not really global. They're actually localized to some class. So what we should be doing is using them from within the class. We're going to encapsulate all the information inside the class that there is. So you can hit here Command Options V, I believe, as the move shortcut. So you can click on that and then say which class you want to move it to. And it'll move it over. And it'll change any references to make sure everything stays compiling. You can do all of this stuff manually. You could cut and paste it and so on. But using the refactorings within the tool will let a lot of the background stuff happen automatically. And it's a lot safer. So now we ended up with this empty interface, totally not needed, and we can delete it. So that is an example of minimizing moving parts. Now if this were a new code that we were writing from Scratch or a new feature that we were adding, the concept behind minimize moving parts is to not build that constants class in the first place unless we actually needed it. So these are the constants that got moved over. And you can see that the split reg x and unknown and so on are there now. And so we can commit that change. So in the front you might be able to see I'm using a git add dash capital A. And what the capital A does is that it adds all of your changes to the change set, including things that have been deleted. So normally you would have to do git rm and then the file name. So we're going to now move on to revealing intent. So revealing intent is about making sure that you're doing what you say you're doing. In this case, we have a file utl class and there's a comment there that says this resource facade supports outbound file operations. I don't really even know what that means. That's a very strange description of this thing that seems to be doing things about looking up file names and moving files and so on. So the comment is essentially meaningless if anything that's confusing us. So the best thing to do in that case is delete it. We don't want to be confused. We're also using version control as I assume everybody is. So these author tags are really redundant and useless and also this one says author is admin. So we delete that. It provides no, it's not revealing any sort of intent. We have a comment there that said this is a constructor. Anybody who programs even a little bit knows that that's a constructor. We don't need to tell somebody that's a constructor. We move on to this next method. It's called setup file. And the comment for it says it gets the CTX main file name from split file name. It says it takes a parameter of name, which is the file name, and returns a file name. So a lot of this is extremely unhelpful. It's certainly not providing any additional information. And if anything, it's more confusing. So we want to try and make this clearer as to what exactly is happening. And we want the names of the things in the application. So the names of the functions, the methods, the classes to accurately explain what's actually going on. So we don't actually even need the comments. So in this case, instead of setup file, we can just rename this to something that's a little bit clearer. And since we have an explanation there in the comment about what this thing is, we can just use that. So in this case, we'll call this retrieve something. Let's see. So I think retrieve CTX file name from split file name. And so now we've got the explanation of what the method actually does is ingrained in the method. Now, it looks like kind of a goofy file name. Excuse me, the method name is very long. And so that might point to a problem with the design of our application. And so that's something we can start looking at later. And now that we've encoded that within the method name, we no longer need that comment. The only time you need to worry about these comments is when, if you're publishing an API for some, again, if you're for a third party or so on, then it's important to have these Java docs and things like that. But in general, for internal code, if you're writing a comment for something, it probably means you didn't explain it well enough within the code itself. Because most of the time, we'll just overlook the comments. And that's why even in our IDE, they're kind of dimmed out, right? So feel free to move up if you want, if it's a little hard to see from back there. OK, so rename vigorously. So this, again, is about revealing intent. So here I'm just going to pause it for a second. So we've got two methods there. One was called p path. And the next one underneath that, which you can't see it right now, is called, let's see. So it was called p path. And the other one was called proc date. So what we're going to do here is, it just seemed weird that one was getting a thing called a processing file path. And the other one was process date. And the names didn't really match up with those things. So we're just going to make it a little bit clearer by calling them the thing that they're actually getting. So this one is processing file path. The next one is process date. And that kind of leads you to the question, when you see it listed out this way, why is one called processing file path? And why is one called process date? Shouldn't it be processing date or something? There's some sort of mismatch there, which was hard to see before, because it was just p date. And so it was confusing. And again, so we're trying to reveal intent to make things clearer. So there's a lot more renaming that you can do in this. And anytime you see something that is named poorly, it's a good thing to change. Again, those are very safe changes because they don't compile time, at runtime, the computer doesn't care what things are called. Those are already stored in the JVM. So the next section here, we're going to be doing some extract method. And you can see in this particular method, there is a bunch of stuff about getting the log file name. So it's generating those log file names from some parameters that are being passed in. This whole section of if statements is all about that. So what we're going to do is we're going to take all of that and we'll use our nifty extract method refactoring. It's one of my favorite refactorings. We'll extract that out. This MDC thing down here is about, it's called a map diagnostic context. It's for logging. Not that important for this, but some people ask about what that is. So we're going to extract this. I believe it's command option M on the Mac, at least. So we'll call this method set up log file, because that's basically what it's doing, it's setting up the log file. And we'll pull that out into a new method. So when you have a method that you have to scroll through and you can't even see it on one page, that's a good sign that it's too long. In general, I like to keep my methods about five lines or less. And below that, so there's that log file name thing. And then there's something about checking the file if it's blank or not. And then there's a whole section below that that's about moving the file. So this is all moving the file in different scenarios. And so we're going to extract that whole thing into a method called movefile, or movefiles. So now this method is looking a little bit more under control, and it reads a little bit better. We can see some setup stuff here, some stuff about getting the log file name and then moving the files. That was hard to tell what this actual method was doing before when everything was listed out as one long thing. So again, all of this is happening so that we can get ourselves ready to add new features. So we're going to move on to rule number one, which is passes all the tests. So we're going to talk about characterization tests. These are different than, characterization tests are different than regular unit tests in that we're not trying to find and remove bugs. We're not looking for errors. Characterization tests are simply trying to capture the current behavior of the system. Regardless of whether it's working or it's working how we think it should or not, we don't care. Because this is code that's running in production presumably, and we just want to find out how it's currently working so that when we make changes to it, we don't have it do something else. Even if there's something else, it's something we think is better. First thing we're going to do is we create a new JUnit test, put it in our little test folder. And so what's the first simplest thing that we can try and test is what we're trying to figure out. So there's this line here on the top that talks about whether a file path that's being passed in is blank or not. So that's the first test we're going to write, because we want to just go through and accurately capture the behavior of that method. So our first test is called blank file path. We run the test, make sure it fails. It should fail, since it says fail in there. If it didn't fail, then we would know that we're running the wrong thing or something like that. So we always want to make sure our tests fail before we do anything. So first thing we do is we create an instance of that class, call the method, and then put an assertion. Those are the three steps of a test. Setup, arrange, so that's the setup part. Act, which is do the thing. And then assert, which is verification. So we're just going to set up all. This is a very common pattern when you're doing characterization tests, is this type of thing, where you just call a method. You don't necessarily even know what it's going to do. But you just start filling in the test until it is in a runnable state. Because oftentimes, these legacy code bases are not they weren't built to be tested. They were built in some other way, generally. So they're not easy to write tests for. So we just kind of do the best we can. So we're going to put an assertion here. We're going to assert that the log file we're getting back from this setup log file method is something. We're not going to try and figure out what that is. We're just going to run this test. Whatever comes back is what we're going to put in there and just capture that this is how it works. Additionally, this thing does something with this map diagnostic context. So we're going to try and capture that in the test as well. So it's putting some values into there. So we're going to assert the log file. And we're going to assert that the map diagnostic context is whatever it is. So let's see. So we have these parameters. So we need to set these parameters some values. So we'll give them some sensible defaults, nulls, dates, et cetera. Run it. And we get a null pointer exception. So our null pointer exception is because we are passing in a null something, I believe. So we're passing in one of these job execution objects, which is null. Obviously, we set it to null, so that's why. So we need to define it. This is a spring object. And I don't know that we want to try and construct them. So they can be trying to manually construct these spring context objects can be painful. So we're going to create a mock object. So this is using mojito. And the way you use it is a mock object for anyone doesn't know is just a fake version of an object. So we just create a mock version of this job execution class. And now we can tell it what behavior we wanted to have. So we're still getting a null pointer exception when we run this. By default, if you just set up a mock and don't tell it what to do, it will return you whatever the non-null version of that thing is. So if you called like a get string or something on it, it would return you a blank string rather than a null. So what this is saying is when we call the job execution dot get job instance method, I think it's job instance. So this is how you set up behavior on a mock. So when you call job execution dot job instance, then it should return you a job instance object. That's how we want it to work. So we need to get a job instance object. So we'll create one of those. And we can see that on this line, that job instance object, it is trying to call the get job name method. So again, we're going to turn this into a mock. So job instance is now a mock. And we'll use this when again. So this is, again, a very common thing you have to do in legacy code characterization tests. It's creating lots and lots of mocks. If I was doing this in a new class and doing test-driven development or something like that, and I saw myself having to have all of these different mocks and things, it's a good sign that there's something wrong with your design. So in legacy code characterization, good. And not legacy code characterization, probably not so good. So we'll run this guy again. And we're going to give our job name. It's going to be called Fred for now. We run it again, and it tells us, you probably can't see this. But this says, the assertion is, it says, we expected empty string, but it got Fred 2015, 10, 10. Now, that's arguably a very poor name for a file, but that's how the system currently works. And again, all we're trying to do is capture the current working of the system. Once we have our tests around it, we can always go back and change it if we feel like it. But right now, that's how it works. That map diagnostic context then gave us an error, and it turns out that it's the exact same value. So we'll log that. So now we should have a passing test. So we do. So this test now accurately captures what happens if you pass a blank file path into that method. So the next question is, how do we know what we should be doing next? You can just go in and look at the classes and try and figure it out. My favorite way of doing it is by using code coverage tools. In Eclipse, there's a tool called Ecolema, which you can get from the Eclipse Marketplace or just download, or however you want, like to get your stuff. And this is actually available for pretty much. It doesn't have to be in Eclipse. You can use it in other environments as well. So you install that, and then instead of running the test, normally, you test coverage, run the test. And when you run it, what you'll see is all of the lines that have been executed are in green, and anything that hasn't been executed is in red. So our test, all of it was executed, so that's in green. This is the production code. And the yellow lines mean that it's been partially executed. So there's a branch in that line. And so only part of the line has been executed, only one of the branches. And the red parts have not been executed at all. So we can look at that line. It says that only half was executed, so we want to now test the other branch of that line. So we can kind of go down that path. So we'll commit that first test. And already now, our system is better, because we are no longer, we have tests around something. So even if we were to go back in and change that little part, we would be ensured to not break it. All right, so our next thing is to not repeat ourselves. So we are going to start the not repeat ourselves segment by repeating ourselves. So we're going to duplicate that entire test. We want to write our second test. We don't know what parts of this test are going to be duplicate yet, so we'll just start by just duplicating it. And we'll unduplicate it after the fact. So we're going to test a non-blank file path now. So we know how a blank file path works. And so we're going to call this file path slash temp slash blah dot text. So that's the file path we're going to use. That is not blank, so we'll run it. We can run our test now and see what happens. This one says it expected, since we copied it, it has our old explication in there. But the actual result was supposed to be just blah dot text. So it looks like it strips off the path and just leaves you the file name. So that's how that works. Run it again. And it gives us same type of expectation failure. And that is the map diagnostic context is also blah dot text. So it seems like these two values kind of go hand in hand, this log file and the input file name. So we run that. We now have two passing tests. And that, you can see the first line of that method now is fully covered. We're covering both branches. So we are reasonably certain that if we were to make a change in that first line, it would not break. We would know if we broke something, right? OK, so you can now see that there's some duplication here. So the way we'll handle that is by moving things into a before method. In JN, these are things that are run before each method or each test. So we can move any of the duplicated stuff up into there. So we'll move that job execution listener and some of the mocks up into there. We want our tests to be, it's very hard when we look at those tests as they were to see what the difference was between the tests, because the tests themselves were fairly long. And so it's hard to see, well, what exactly is different about these tests? And in reality, there was really only one line that was different. It was just that file path line. One had a blank and one had a slash temp in it. So we move that stuff up into the before and change those into instance variables, and then we can start sharing it. So that first method is looking OK. So now that we've got some things out of there, we can move things out of our second test as well. Clean some of that up. And it's a little bit better. You can still see there's some duplication, and we'll come back to it and clean up some of that duplication, I believe, in the next segment. So tests still run. Everything looks good. We can also see that there's some duplication in these assertions. We have two assertions, and this is really the same assertion. And they're kind of telling us the same thing. So we see that we talked about the log file and the map diagnostic context having the same value. So what we'll do is this is a common pattern when you're trying to extract something with a parameter. We first extract the variable that is different between the two instances. So down here, it's blah.txt. Up there it was spread. We extract that out into its own variable, and that's just refactoring with an eclipse. And then you highlight the two lines that are going to be extracted into their own method. And then we can hit Command 1 or Control 1 to extract that method into its own little thing. We'll call that something like extract, or what is it? Assert log file name. So we're going to assert that the log file is correct. And then we can take that variable that we extracted and then re-inline it, so we don't have it just by itself. So that's a pretty common pattern to extract the variable, extract the method, and then re-inline those three steps. So we'll move that new assertion that we have now down to the bottom, and then we can use the assertion in the second test as well. So we assert log file is blah. Now it becomes, again, clear what exactly we're asserting. We don't have to read through so much on those tests. There's a lot more details about what a assert log file is. We can always go down and look in that method. But it's not really relevant to the test itself. Run our tests again. Make sure they still work, which they do. It's always a good idea to frequently run tests because every little change that you make can cause problems. So check that one in. OK, so now on to the next line. So we have our next line also as a ternary operator. It's checking to see whether this job name is a particular value. And each one of those is getting hit independently. So each one is having only one branch of that if statement being hit. So we'll write another test here. And this is going to be the agency job name, agency debt extract. So in this case, we have a job instance, get job name. We're going to say that it will return us a value of agency debt extract rather than just Fred. And we will run that and see what it does because that should do something different. Or we presume it should do something different. Here we go. So it fails. This time it gives us an illegal argument exception. Says the partial must not be null is what it's telling us. And that just happens to be a poorly named error coming out of the Jota time library. So this thing is calling its job parameters dot getDate. And it's trying to get a processing date. What's happening is that that's coming back as null because we haven't set that up in our test. And it is therefore giving us a weird error. So in order to handle that, let's see. So we need one of these job parameters to return us a proper value. So let's set this processing date inside the job parameters object. So job parameters, it turns out, is a spring class again. And there's no way to set a value on it. So maybe we can do get parameters and then set something. But as it turns out, there's no real way to do this. So we can try putting this processing date. But then it wants a job parameter object, which we don't have. And it's just getting more and more complicated to try and set this value. This is, again, a very common thing that happens with legacy code that's not written to be tested or written with tests is that you get into these weird situations where it's hard to actually construct tests. So this is another good time to use mock. So right now, we were creating a new job of just an empty job parameters object. So rather than that, we're going to change that to be a mock instead. And we will now just have it, all we have to do is just say when we call job parameters, get value, let's see, job parameters, get name, get string, something like that. Job parameters, get date, there it is. And we pass in the processing date. So if the code sees that call, job parameters, get date, processing date, then it should return us some value. And we'll just construct a new Jota time local date object. Still a little bit messy, but it'll do for now. And then we can get rid of that other line. So now we're actually returning a value that the test hopefully will be expecting, or the production code will actually be expecting. Now it could also be valid to keep that other test that had no processing date, because that could be a potential scenario that occurs. And even though it threw an exception, it could be something that we want to capture as a characterization test. This one is saying that the name of the resulting file name is agency debt extract null 2015 and 1010. It's probably true that null was not intended to be in there. And so we saw that there is another value that it's trying to get that was an agency ID. So we'll write another mock expectation that says if we make a request for agency ID, then it returns us just ABC. So we run it again, and now it says the resulting file name should be agency debt extract ABC 2015 1010. Again, arguably a pretty poor file name, but it is what it is. So we put that into our assertion, and our test is now passing. If we look back at our code coverage, hopefully we look at our code coverage. Here we go. If we look at our code coverage, we can see now that first if statement is now fully covered. So this could be a tedious process, kind of going through and working your way, but it's pretty unsafe to start going and just changing the code without having this sort of a safety net. So the next step we're going to do is try and refactor our test. So there's some duplication here where we see that this file path value is now being copied as well in all of these tests. So refactoring isn't just for production code. Refactoring is for tests, too. You want to keep them lean and mean as well. So in this case, we're going to move that file path value up to the top level. And we're going to default it to an actual value. And in this first test, where we're talking about blank file path, we're just going to set it to be blank. And all the others will just use that default value. Again, we want it to be clear when we're looking at it what that test is concerned with. In this case, the test is concerned with a blank file path. The other ones we don't really care about the file path. So that all still works. So our next step is to get full code coverage. And don't worry, I'm not going to go slow here. We're speeding it up four times or eight times or something like that. And I don't type this fast. So basically, we're just going through with that same process, looking at the code coverage, checking all the branches, making sure that we're hitting them all. And essentially, what's happening is we're changing that job name that's returning. Because each one of those branches is looking for a different job name. And they have slightly different file names. So again, this is not about fixing the design right now of the application. Because I'm sure all of you can see brilliant ways in which we could improve that code, that production code. And we'll get to that. We're just not going to do it yet. So keep running and adding some of these. We've got seven tests, eight tests. I think we're getting there. We'll run our code coverage one more time. And so it looks like all of our code now is covered by our tests. This again is not necessarily a guarantee that you've hit every possible scenario. We're not testing for nulls. We're not testing for weird characters. There are definitely, you're not 100% sure that you've captured everything. But we're fairly high confidence. So now we feel like we're in a state where we can safely refactor something. And again, all of this is in preparation for actually adding new features. So we'll run our tests one more time just to make sure everything looks good. So we see this first line here is about, it was checking for not is blank and then doing a condition. So I just reversed that into not a negative condition. So it just makes it a little bit easier to read. Next thing we see is that in each of those if statements, we're logging the exact same thing. So we're taking that out and just moving it down to the bottom so we don't have to have all of that duplication. We can see lots of duplication in this method. The next thing we see is that all of these if statements really are all concerned about actually extracting that log file name from these various values. So we extract all of that into a method called generate log file name. So it's inside this method that's called setUpLogFile. So that's actually setting up the file itself. This one is specifically about just getting the name of the file. So we'll get that into its own method. So now it's a little simpler. There's less stuff. We don't have the map diagnostic context and stuff inside this method either. So it's doing less stuff. We want to try and get our methods so they're only really focused on just doing one thing. So we've got all these if statements. If we match any of these if statements, we don't need to continue matching all the other ones. So we just convert those into else ifs. That simplifies that a little bit. And then if we look at this, so we run it and make sure that all still, that was kind of a long time to go without re-running our tests. But we did it anyway. So this thing is actually setting up this log file name and then potentially overwriting it at the end. And so we're just going to rearrange that so that we only set this value once rather than setting it and then potentially overwriting it. So we don't necessarily need that default value. So we'll add that to the last else condition. And then we now have a situation where we're in a situation now where we have all these nice else ifs. Well, they're not nice. But they're else ifs. And then we have a ternary operator in the bottom. So it's kind of a different paradigm. We're switching paradigms in the middle of this else statement. And so we're just going to convert this also into more else if conditions just to keep it consistent and make it easier to read. We want it to be not surprising to the person who's reading this code. There's a part coming up here where I struggle with putting in parentheses properly. So now we're all in else ifs and ifs. I think that might have been it. So it reads a little bit nicer now, even if, again, it's not the best code in the world. All right. So again, now here we've got this value and then we're returning the value at the end. We don't really need to do that. We can just return the value just kind of inline in each of these spots. And some people think that's not a great idea to return out of the middle of a method. In general, I think that it's OK as long as you have nice short methods. Once you start getting long methods and it becomes hard to see that that's actually happening, then it's confusing. But in this case, it's a very consistent pattern. It's very clear and easy to see exactly what's going on. So I think it's OK to do those types of returns. It also means that you have more immutable variables and we're trying to get to immutability if anyone's doing any sort of functional programming or anything like that. OK. So that is now working. And let's see. So we also have this job execution, get job instance, get job name thing that we're repeating numerous times. And we can extract that simply into something that a variable called get job name. So now we don't need to keep checking that every time. So this is arguably quite a bit simpler than the original code that we were looking at. Significantly less duplication. There's obviously a lot more things that you could do to it. But it's, I think, a pretty reasonable improvement. All right. And so we will check that in. So we're committing, this is our final check-in. And I believe that is the last one. So we're now in a state where we've got a set of code that is covered with a good set of tests. And we can now go in, add a new feature around that area. If we were in, if we added a feature that needed to now touch some other code, we would probably want to write some tests around that as well. Again, just capture behavior before starting to change it. That's really the key for that. So I think I may have one minute for a question. Do I? Yes? All right. Anybody have any questions? If not, then that's all I have. And you feel free to come up and ask questions afterwards, or I'll be out in the hallway. Thank you very much.