Refactoring Controller Methods Discussion
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.
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.
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.
I agree with you that teaching methods matter.
It really affects your knowledge base.
Amr N Tamimi
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.
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.
mattangriffel
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.
James Kiesel
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. :)
Grigory Reznichenko
Why not move QUESTIONS to the SMS model? I prefer controllers to be as thin as possible.
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.
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.
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.
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.
This was great thank you! I am confused as to where the @reply.state is coming from. How is that determined?
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.
Kohl Kohlbrenner
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 :(
Sorry, Chris. I'm just realizing that I put this in the wrong comment section.
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?
Gary
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?
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...