Skip to main content

27 Refactoring Controller Methods

Episode 39 · January 15, 2015

Learn how to refactor a complex controller with a bunch of methods into a much cleaner set of code

Refactoring


Transcripts

In this episode, we're going to talk about refactoring a controller that my friend gave me. I stripped out a lot of code that we're not going to cover in this episode, but they had a bunch of these methods inside the controller that sent text messages, so this interacts with the Twilio API, and talks to the user that's passed in the SMS phone number. They have basically all of these methods that do the same things, and they all initialize the Twilio REST client, they set the phone number and they set the phone number it's sending to. Then, each of them has a different body of text that they're sending. I thought it would be nice to take a look at this and see how we could refactor it.

The first thing that is easiest to do is to pull out a method called client, and the way we're going to do that is we're going to take this and the reason why we call the client is because we called the Twilio REST client, "client". We do that, we can get rid of this variable here, and return the Twilio REST client, and that means that each and every one of these methods doesn't need these three lines of code anymore, and by removing that line and naming the method the same as this local variable, we can simply refactor that out, and then not have to do anything and not change any of these methods except for deleting those lines. These lines will function exactly the same as they did before, and that's pretty cool. We could do the same thing with from and to, but especially in the to case, we already have the send to variables, so why don't we just update that and get rid of this local variable, because we only use it the one time in each of these methods, so we can delete these, and go through each of these methods, and shorten them just a bit, you can see that originally, the developer was testing these and we got something working, so let's just copy paste this because the first time worked and we can trust that the rest of these is going to work. That's not an unreasonable mindset to have when you're developing something, it's like a tool like you've never used before.

All of these use the exact same phone number to send from, so let's just actually hard code it here, because we're never going to need to change that, and if we do, we can use an environment variable or something in the rails application config or something. WE can change this and leave it that way, and if we ever do need to update it, we can do that accordingly. I'm going to go through and update all of this as well.

Now we've got this shortened quite a bit, and you can start to see that these methods are a lot more deliberate now, so each of them simply says like: Hey, let's create a text message from this number to this number and the body, and because of the way that this is visually, you can also see that there's really only one method call inside each of these methods, even though you see like hash variables, the keys and the values. You can tell that they're part of this method call because of how they're tapped in. That's pretty nifty and that helps us a lot. One thing we can do with these two is the account_sid variable and the auth_token variable are unnecessary, they don't really provide us any benefit, because they're only used once, and we might as well just put them there. If we delete that, we can save another couple lines of code, and add a little bit more clarity not adding those variables. We can also think about making a Send SMS method

def send_sms(to_phone_number, body)
    :from => "+15128618455",
    :to => to_phone_number,
    :body => body 
end

Having the phone number and declaring that it's the phone number you're sending it to makes it a whole lot clearer so you can see it at a glance. Now we can update each and every one of these methods to say

def get_first_nm(sendto)
    send_sms sendto, "Welcome!"
end 

Now that we've done that, you can see that all of these lines are becoming a lot clearer. You can glance at them and you can see we're going to send an SMS, we don't really know that that means, we want to probably update all of the sendtos to to phone number, but we do know at a glance like what message we're going to send them. Let's actually just grab all of these change sendto to to_phone_number, and then, now we can see send SMS to this phone number, and this is going to be the message we send. That helps a whole lot. We can also see this default response does a little bit of database logic, it pulls a response out of the database, and then sends an SMS to that phone number with that custom body, and we can refactor that a little bit just by cleaning this up and then we have this tabbed over little piece of logic here, and this is going to help us by saying: Ok, we can see that the body is set just one time, but actually, why don't we take this a step further. Why do we even need this body variable? Maybe we do, maybe this body variable is rendered inside of a response somewhere, but if it's not, why don't we just make a method called default_response. These clearly don't use any local variables, because we didn't set any before this line, so we can actually just take this out and paste it into this own method. If we do that, then we know what the default response is, and we can get rid of the body variable so long as we're not using it in the HTML or XML response or something like that. This is the way that I definitely prefer to do this because then we can set our text here and send_sms to_phone_number and obviously it's going to be the default response. We don't have to care here how that works, we just have to know that as long as the default response gives us what we want, then we're good, we don't have to worry about that whatsoever. That's cool.

Now we've refactored this quite a bit, but you can tell that there's a whole lot of things that are still almost unnecessary you might say, like we've got the replies and the controller knows how to talk to Twilio and it knows how to send an SMS and all that, like it seems to make more sense if we actually make an SMS class that controls how those works, so if we think of that, we can make an SMS class and we can say: Ok, this SMS class is going to be the one that knows how to send messages, and we'll use that instead of making this little method here, but the way we did this, and refactoring into little helper methods, we can start to see the areas of responsibility. You can see that these two methods are extremely related to how to send an SMS. The rest of these are just simply sending one. They don't really care, they know what text to send and all of that, so we should actually pull these out into their own object.

I've created an SMS class here that we can pull that into. Let's just grab the client and send_sms method and paste them in here. Now we have instance methods that allow us to send that to a phone number, but why don't we also create an initialize method that accepts the two phone numbers so we can save that, and then here instead we'll send to that phone number. This allows us to update our code here with the idea of: Hey, there's an SMS thing, and we would want to send it to the phoen number, and then we can rename to send instead of send_sms. This allows us to create a new SMS, or the idea of: Hey, we're going to send an SMS to this person, and then the send method will actually send the text to them. This allows you to update these and they read a lot better, or at least I think they read better. This way, you have the concept of sending an SMS. This doesn't really send an SMS. Twilio is the one that really does it, but your application has the concept of sending the SMS, so this also allows you to do some neat things. Maybe your introduction is going to be a little bit longer than one message, so in this case, what if you had "Welcome to BringUp! Let's begin" And that was just a single message, and then what if you had another SMS that you wanted to send to the same phone number? You could simply do this, and this SMS class would allow you to send two messages to the same phone number because it saves the phone number and you could use this sort of on a longer basis. You could reuse the SMS object multiple times to send them multiple messages.

I'm going to take this back a notch and we're going to go back to calling send here on the single message, and then we now can see that all these methods are interesting, right? I mean they all have a name, and then they all have an associated string that you want to send. You can imagine that there is some piece in the controller that says:

if "first name" 
    get_first_name
else "last name"
    get_last_nm 
end 

You can imagine there's a piece of code in the controller somewhere that I didn't include that calls these methods. What's neat about the way that we've refactored this down is that you can see we're always creating a new SMS, but the message is going to be different each time, but we also have the name. Somewhere we know that we won't ask for the first name, so imagine that there's an @reply.state and that state could equal "first nm". If we had some hash of keys and values and we had "first_nm" and the message we want to send to people in that state is this, and then maybe we have "last_nm", and the message we want to send them is: "What is your last name?" and we can update this to say: "What is your first name?", and then we could start pulling all of these out, and we can have the child name, and the questions at those states in time. The way we could do that is we could either create a questions constant and set it equal to that, we could have a method that returns this or whatever. I'm going to do a constant because they should always stay the same, and then we could take these methods and we could actually get rid of them almost completely. We could have a method called send_next_sms and this will look at the reply state, and as we mentioned before, this would be like first_nm and then, here we could retrieve that question and we can just say

def send_next_sms 
    QUESTIONS[@reply.state]
end 

This would return us this string, we would get that back, and then we could just simply send the SMS, so we could simply say

def send_next_sms(to_phone_number)
    SMS.new(to_phone_number).send QUESTIONS[@reply.state]
end 

That would allow us to delete all of these methods, so as long as we moved all of these properly into this, so we could also do like-- These methods don't have the same names but it doesn't matter, as long as we know that let's make one called "sign_off", and that's going to be the state that we want to send to the user. Then we can name these accordingly so long as we remember this, then we can look up the message all the time, and we can delete all of these. The one thing we can't delete is the default response. What if this reply state doesn't exist in there? So what if the state is something wrong? Maybe there is nil or something like that, in that case, we can switch this square brackets with the fetch, and we can send the default response instead. Fetch will look for the state, and it will look up the question with the name of state, so it will try and do that, but if there's nil, and there's no nil equals whatever, then it will fall back to the default response. We can even delete the default response by doing that, and we can add states into our system at any time just by simply adding a line in here and saying "relationship" => "What is your relationship?" You can simply refactor this down to a hard coded list of names and questions or messages, and then take all the duplication of each of those methods and have one that is smart about how it works and delegates directly to the proper message. This way, we can refactor our code down really cleanly so that there's really nothing that can go wrong with this. If you have an invalid state, the user will get the default response, and an SMS will always be sent. We've taken out all of the if statements, except for this one, and this one can't really be removed, because we have to check to see if there's a parent and it has a response, if it doesn't then we can't use it. In this case, we're always going to need a little bit of logic. One thing we could do is we could combine this into something but it's not much to worry about. What we've done is we've removed these separate methods and all the logic around that, and simply replaced it with one that delegates to this hash of questions.

I hope that's helpful for you, it's basically simply taking these lines of code, splitting them up into methods inside the same file, and then rethinking about what concepts or what groupings do these methods have? Definitely sending an SMS and connecting with Twilio belongs and something, but the rest of these actually belong together. There's something that these questions and sending the next question all belong to. We can even do

def next_question 
    QUESTIONS.fetch(@reply.state, default_response)
end 

Then we can always send the next question. These probably belong is some sort of model, maybe these are questions for your user, and so this way you could always look up the right next question for the user and so on. I actually prefer this extra method here so that we can write tests to make sure that hey, this user of the replies in the current state, then we can look up that, and then we can write our tests to make sure that: If you're on the first name we should get this message and so on. Then, we could write a test just simply to make sure that SMS gets created accordingly and so on. For testing purposes, we've broken things down into very very very short things, and removed any of the duplication, as well as any of the if statements in there, because now, the hash defines all of the logic of finding which question goes to what. We don't need any if statements, and that makes this code very very clean, simple and testable

Discussion