Ruby Science: Extract Class | Upcase

Upcase
1
Show notes: Melanie joins Joe again to discuss the classic refactoring step: Extract Class. Learn how to safely split up a Large Class in small steps, keeping your tests green as much as possible while Melanie demonstrates this technique live. Af...
This is a companion discussion topic for the original entry at https://thoughtbot.com/upcase/videos/ruby-science-extract-class
2

Great episode guys; @benorenstein that was creepy :stuck_out_tongue:

@melanie @jferris I have a couple of questions though.

  • Why didnā€™t you create a recipient list object instead of making it a method inside the SurveyInviter?
  • I know this would be like arbitrary but why make the parameters list an attributes hash if there were just like two parameters?
  • Isnā€™t better just to add an attr_reader from the start so you donā€™t have to be adding @ like everywhere; even if you start with private accessors at first?
  • Isnā€™t the code in the controller breaking tell, donā€™t ask and making decisions based on the SurveyInviter state?

Thanks in advance for all your answers,

3

BEN IS BACK! YAY I CAN BEGIN WATCHING AGAIN STARTING NEXT WEEK! :smile:

But seriously, love all the eps this one was great.

Cheers!

  • Dain
4

Hi @Rafael_George,

I agree about attr_readers, I always use them and make them private if need be.

I immediately thought that as soon as the valid? method was added to the SurveyInviter class. I think itā€™s a good idea to move in smaller steps though and getting to a state where you can see that it violates Tell Donā€™t Ask. We need to be careful to keep in mind Separations of Concerns and making sure that the controller logic is still in the controller. So weā€™d still need a query method, something like success? in order to redirect or render errors. I normally create Request/Response Objects that look like this:

response = InviteToSurvey.call

if response.success?
  # happy path
else
  @errors = response.errors
  render "new"
end
5

Why didnā€™t you create a recipient list object instead of making it a method inside the SurveyInviter?

Itā€™s generally good practice to do one refactoring at a time. When extracting a class, itā€™s not unusual to find out you can actually extract two! In the example app in Ruby Science, we ended up extracting several classes for this interaction.

I know this would be like arbitrary but why make the parameters list an attributes hash if there were just like two parameters?

One big difference between one parameter and two is that you can forget the order with two. Using named parameters makes the order unimportant.

Isnā€™t better just to add an attr_reader from the start so you donā€™t have to be adding @ like everywhere; even if you start with private accessors at first?

I think there are valid points on either side of the argument:

  • Using attr_reader keeps the abstraction flat and prevents churn if you need to change it to a method later.
  • Using @ directly tells the reader explicitly that itā€™s an instance variable, so you donā€™t have to wonder how it works.

In all my time in Ruby, Iā€™ve never found that either one of these approaches has much effect on readability or ease of change, so Iā€™d use whichever style appeals to you aesthetically.

Isnā€™t the code in the controller breaking tell, donā€™t ask and making decisions based on the SurveyInviter state?

Sometimes Separation of Concerns (MVC) and Tell, Donā€™t Ask donā€™t play well together. If you want to keep view/controller logic separate, it means youā€™ll sometimes have to ask the model layer questions and then make decisions on their behalf. Otherwise, youā€™ll have to peform rendering and other logic in your models. MVC has proven to be a win in most applications, but itā€™s not universal.

2 Likes
6

@aaronmcadam yeap; Iā€™m also using that approach with the response objects; quite handy.

7

@jferris Thanks for all your answers; and thanks for the episode again. Keep them coming :slight_smile:

8

Lightbulb moment. This is awesome!

1 Like
9

It there a reason that you did not create tests for this new object? While the methods that we moved to the SurveyInviter had been private, and thus did not need tests, the SurveyInviter methods are public.

1 Like