Skip to main content

29 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


Gravatar
Tito Brahmanto (20 XP) on

I'm still downloading this episode right now, but I want to answer your question in your email 'Should we do more refactoring and talking about how to write clean code in the future?'. And my answer is Yes. Thanks.

Gravatar
Chris Oliver (167,500 XP) on

I'll be sure to cover more topics then! Feel free to email me any examples of code you'd like to see refactored and I'll see what I can do.

Gravatar
Jay Killeen (1,580 XP) on

I'm a big yes too. Possibly in a way that directs towards some of Sandi's 'guide/rules' that they keep talking about on Ruby Rogues and Giant Robots Smashing Into Other Giant Robots (ie breaking big things into little things, explaining the whole methods sending messages between each other).

Another idea for you would be to have a 'these are the specs to go with an episode'. So when you do a cast on Twitter omniauth login etc we have another cast showing how to test it. From a beginner point of view that's the biggest gap in my knowledge. Everyone says 'test first' but it is hardly ever tackled like that in the teaching methods. I know it could be cumbersome so a seperate episode might work.


Gravatar
Amr N Tamimi on

Hey, I just watched this screencast and I have a comment about using send method.

Here, you are overriding Object#send. send is a reserved method for Object in ruby. So when you do send on any object instance it will try to find a method for that instance.


class Test
def initialize(name)
@name = name
end

def greet
puts "Hello #{@name}"
end
end

s = Test.new("Amr")
s.send :greet #=> "Hello Amr"

So for best practice, I suggest not to use send here.

Gravatar
Chris Oliver (167,500 XP) on

Whoops, you're totally right! Thanks for the comment. That was my bad. I knew I was probably overlooking something calling it "send".

Definitely a problem when you don't have the full Rails app to test your refactorings against.

Gravatar
Amr N Tamimi on

I appreciate you for building this and recording such screencasts :D

Gravatar
David Van Der Beek (20 XP) on

Looks like you also have an extra param in that send method. It doesn't need to accept the to_phone_number since you have it in your initialize method. Also, you probably need to pass to_phone_number to your default_response method.

Gravatar
Chris Oliver (167,500 XP) on

Oh yep, you're right. It's easy to miss stuff like that when you can't actually execute the code.

Gravatar
mattangriffel on

Fortunately Ruby anticipates people overriding the send method (a number of standard libraries do this) so they added a __send__ method which can used to be on the safe side.


Gravatar
James Kiesel on

You'll need to pass `to_phone_number` through to `default_response`, because you're looking up the Parent by it.

Also, definitely no need to call `Parent.where` three times; call it once and store it so you don't need to hit the DB three times to retrieve one record. :)


Gravatar
Grigory Reznichenko on

Why not move QUESTIONS to the SMS model? I prefer controllers to be as thin as possible.

Gravatar
Chris Oliver (167,500 XP) on

That's definitely an option. I prefer not to because the QUESTIONS are related to business logic regarding some type of model and the SMS class just purely contains logic on how to send an SMS. This way it's separated from other concerns and is purely on its own. You could extract this out into a library if you wanted to share it between apps for example and you wouldn't have the QUESTIONS and other dependencies.


Gravatar
Daniel Clark (20 XP) on

I really like the way you've demonstrated this. I need to add the .fetch() method to my day to day programming. I've never used the default option. I tend to do an or for my default: || :my_default

And I admire your VIM skills. A few more demonstrated tricks and I might just learn it. I haven't converted to the editor wars. I watch from the sidelines with the Pico/Nano editor.

Gravatar
Chris Oliver (167,500 XP) on

Fetch is really great because it can throw an exception when your key is missing rather than just calling undefined methods on nil later. It's great for that but it also is a little cleaner than the "or" like you mentioned.

Check out Janus (https://github.com/carlhuda.... It's a collection of Vim plugins that give it some pretty great defaults and can save you a lot of time learning Vim at the beginning.

Gravatar
Jared Smith (210 XP) on

https://github.com/skwp/dot... is another great set of dotfiles to get you started with Vim. It's saved me ton of time, and has nearly all the plugins I need minus a few gems here and there.

I highly recommend learning Vim it can make editing code insanely fast.


Gravatar
Matthew Orahood (20 XP) on

This was great thank you! I am confused as to where the @reply.state is coming from. How is that determined?

Gravatar
Chris Oliver (167,500 XP) on

Reply would be a database model somewhere with a "state" attribute. I didn't include that part here because I didn't have the full code to begin with. You can assume it's used to keep track of the conversation and what "state" it currently is in.

Gravatar
Matthew Orahood (20 XP) on

Oh ok thanks! That makes sense now, you guys make a quality product. I am seriously considering a pro membership to help me with developing my open source app. Thanks again!

Gravatar
Chris Oliver (167,500 XP) on

Glad you're enjoying it Matt! :) Hit me up if you have any ideas for screencasts or ways to improve the site!


Gravatar
Jared Smith (210 XP) on

Awesome demonstration of refactoring.


Gravatar
Kohl Kohlbrenner on

Chris Oliver I modeled my controller a little different than yours and I am successfully saving the like to the database, but I am getting an undefined method error on first_or_create! before the redirect to a new page. I will post my controller action below. :

def create

@like = Like.where(user_id: current_user.id, recipe_id: @recipe.id).first

if @like.nil?
@like = Like.where(user_id: current_user.id, recipe_id: @recipe.id).first_or_create
@recipe_was_liked = true
redirect_to recipes_path

else

@like.destroy
@post_was_liked = false
redirect_to recipes_path
end

end

Figured out the problem, once again! All I'm doing now is polluting your comments section :(

Gravatar
Kohl Kohlbrenner on

Sorry, Chris. I'm just realizing that I put this in the wrong comment section.

Gravatar
Chris Oliver (167,500 XP) on

Haha no worries! :) Feel free to ask questions on the forum too.


Gravatar
Praveen Perera (20 XP) on

Why not move the logic in the default_response to the model. That way you could name what each part does and make the code more readable?


Gravatar
Gary on

Hey, there. I like the screencast. It was actually really helpful and I would have never though to use an SMS class.

But can someone explain what QUESTIONS[@reply.state] is doing? Specifically can you give me an example of how and where @reply.state would be defined so that QUESTIONS[:first_nm] would be eventually be interpolated?

Gravatar
Chris Oliver (167,500 XP) on

QUESTIONS is a list of all the questions to ask in the various states of the application. When you move to the next state, you can easily lookup the question from the QUESTIONS hash that you need to send out. What happens is that when the user sends a response with their first name, the @reply model gets it's state attribute changed over to "last_nm" and then it knows to ask for the last name. Each time a response is saved, it updates the state to the next one. I made a couple episodes on state machines and the state machine that can help you wrap your head around this part:

https://gorails.com/episode...
https://gorails.com/episode...


Login or create an account to join the conversation.