# Saving posts, then tweeting them

Here's a part of my controller and it's getting quite lengthy (the code works).

Would this code slow down the performance of my website? Can it be cleaned up and be written more efficiently?

def create
@post = current_user.posts.build(params[:post])

if @post.save
if @post.verify
UserMailer.verify_email(@user).deliver
redirect_to root_path
else
flash[:success] = "Posted"

if @post.content.to_s.length > 140 and @post.image.present?
redirect_to root_path
elsif @post.content.to_s.length > 140
redirect_to root_path
elsif @post.content.to_s.length <= 140 and @post.image.present?
redirect_to root_path
else @post.content.to_s.length <= 140
redirect_to root_path
end
end
else
@feed_items = []
render 'static_pages/home'
end
end


## migrated from stackoverflow.comApr 10 '13 at 7:13

This question came from our site for professional and enthusiast programmers.

Roughly:

twitter = Twitter::Client.new

img, max = @post.image ? [@post.image.path.to_s, 120] : ["", 140]

redirect_to root_path


All the Twitter stuff shouldn't be in the controller, rather in a utility class. This makes testing easier, makes plugging in implementations easier, makes extension easier, and so on.

Also, truncate should be already adding the "..." if truncation was necessary.

Personally, I prefer Amit's answer. IMO it's easier to read and more obvious than mine.

I would probably tighten it up a bit after an initial review to something like this:

s = @post.image ? "#{@post.content.truncate(120)}... #{@post.image.path.to_s}"
:    @post.content.truncate(140)


It's formatted to highlight the similarities between the two, another option is to use string interpolation again, but it seems redundant in this particular case.

(Obviously the string creation would go into a method in some sort of support decorator etc.)

All the above is untested, but probably close.

• nit-pick: .present? can be safely removed. – tokland Apr 10 '13 at 9:40

You could create a method on your post model that converts a post to a tweet. If you do that you can clean up your controller a lot:

Model:

class Post < ActiveRecord::Base
def to_tweet
if image.present?
"#{content.truncate(120)}... #{image.path.to_s}"
elsif content.length > 140
"#{content.truncate(120)}... more"
else
content
end
end
end


Controller:

def create
@post = current_user.posts.build(params[:post])

if @post.save
if @post.verify
UserMailer.verify_email(@user).deliver
else
flash[:success] = "Posted"
end

redirect_to root_path
else
@feed_items = []
render 'static_pages/home'
end
end

• prepate > prepare. Otherwise this would definitely make the code a lot cleaner. It wouldn't necessarily make it any faster, though the OP probably needn't optimize prematurely. – Finbarr Apr 10 '13 at 2:37
• can you show me what the method in the post model would look like? that would help me out a lot – user2159586 Apr 10 '13 at 2:37
• @user2159586, updated my answer. – Mischa Apr 10 '13 at 2:41
• I'd argue that posts shouldn't know about Twitter. – Dave Newton Apr 11 '13 at 16:19

Here's how you could simplify things, maintaining the existing logic. However, the @twitter.update logic which depends on the whether or not the @post has an image present should happen in a separate (non-ActiveRecord) model. I find it always helps to write your own wrapper class around whatever external API client you use. This has at least three benefits, (a) it allows you to expose precisely the interface you want to the rest of your application -- no more, no less; and (b) it allows you to unit test things a little more easily, and (c) most importantly, it minimizes the coupling between your code and this external library so if Twitter decides to totally change the client API you only have to make changes in one place.

def create
@post = current_user.posts.build(params[:post])

@feed_items = [] and render 'static_pages/home' and return unless @post.save

if @post.verify
UserMailer.verify_email(@user).deliver
else
flash[:success] = "Posted"

if @post.image.present?

• Yes, it's enough to just check image.present?. However, I don't think the one-liner with @feed_items will work. render won't stop the rest of the code being executed, resulting in a "Render and/or redirect were called multiple times" error. – Mischa Apr 10 '13 at 3:05
• That's a good catch. I could stick on a and return before the unless, but it was already starting to feel a bit weird to me. I'll put it in for correctness, but the real right answer for the OP is to extract a bunch of the logic to other classes. – Amit Kumar Gupta Apr 10 '13 at 3:10