Skip to content

Instantly share code, notes, and snippets.

@dhh
Created March 3, 2014 20:31
Show Gist options
  • Select an option

  • Save dhh/9333991 to your computer and use it in GitHub Desktop.

Select an option

Save dhh/9333991 to your computer and use it in GitHub Desktop.
class GroupersController < ApplicationController::Base
def create
@grouper = Grouper.new(leader: current_member)
if @grouper.save
ConfirmedGrouperEmails.new(@grouper).deliver
AssignBarForGrouper.enqueue(@grouper.id)
redirect_to home_path
else
render :new
end
end
end
# app/mailers/confirmed_grouper_emails.rb
class ConfirmedGrouperEmails
def initialize(grouper)
@grouper = grouper
end
def deliver
LeaderMailer.grouper_confirmed(member: @grouper.leader.id).deliver
WingMailer.grouper_confirmed(wings: @grouper.wings.pluck(:id)).deliver
AdminMailer.grouper_confirmed(grouper: @grouper.admin.id).deliver
end
end
@adomokos

adomokos commented Mar 4, 2014

Copy link
Copy Markdown

Why did you make ConfirmedGrouperEmails statefull? Could you just make the #deliver method a class method and pass @grouper to it as an argument?

I guess the real question is: what is your thinking about making objects statefull vs. stateless?

@jonahx

jonahx commented Mar 4, 2014

Copy link
Copy Markdown

It's not stateful in any meaningful sense. Yes, it has a member variable, but there's no way to mutate it. It is essentially nothing more than syntactic sugar over deliver(grouper)

@tel

tel commented Mar 4, 2014

Copy link
Copy Markdown

I've never understood this style—though it appears popular. There's seems to be no improvement over deliver(grouper). I feel like it's just making use of the fact that you built a class to organize this component so you "may as well" use the initializer.

That said, I think if it were truly just a function it'd be a bit strange since in Ruby you kind of rely on classes to provide semantic grouping.

@clemens

clemens commented Mar 4, 2014

Copy link
Copy Markdown

The advantage is quite simple: At some point there might have to be some kind of state – this is where you'd then either convert all consumers of your ConfirmedGrouperEmails API to use the instantiated version or provide a kind of wrapper to have the class method create an instance and deliver it by calling ConfirmedGrouperEmails.deliver(grouper). This is not premature optimization but a real issue.

Simply put: Don't use class methods unless you really have to. Ruby is an OOP language so use OOP.

@jonahx

jonahx commented Mar 4, 2014

Copy link
Copy Markdown

In this specific example you're right, there's no improvement. But if you were to add other methods beyond deliver, which also make use of @grouper, then you get the improvement of not having to pass the parameter around constantly. It's not really any different than preferring "hello".upcase to something like uppercase("hello")

@dideler

dideler commented Mar 7, 2014

Copy link
Copy Markdown

@tel I think the improvement is readability: .deliver(grouper) comes across as delivering a grouper, while .deliver is read as simply delivering whatever it was called on.

@shime

shime commented May 6, 2014

Copy link
Copy Markdown

@eprothro

Copy link
Copy Markdown

@shime 👍

Put another way, passing the argument to a class method is not "exemplary" (a la Sandi Metz). So future development efforts (often by other devs) are likely harmed, since this pattern resists refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment