1
\$\begingroup\$

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?
        @twitter = Twitter::Client.new
        @twitter.update(@post.content.truncate(120)+"... " +@post.image.path.to_s)
        redirect_to root_path
      elsif @post.content.to_s.length > 140
        @twitter = Twitter::Client.new
        @twitter.update(@post.content.truncate(140))
        redirect_to root_path
      elsif @post.content.to_s.length <= 140 and @post.image.present?
        @twitter = Twitter::Client.new
        @twitter.update(@post.content.truncate(120)+""+@post.image.path.to_s)
        redirect_to root_path
      else @post.content.to_s.length <= 140
        @twitter = Twitter::Client.new
        @twitter.update(@post.content)
        redirect_to root_path
      end
    end
  else
    @feed_items = []
    render 'static_pages/home'
  end
end
\$\endgroup\$

migrated from stackoverflow.com Apr 10 '13 at 7:13

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

6
\$\begingroup\$

Roughly:

twitter = Twitter::Client.new

img, max = @post.image ? [@post.image.path.to_s, 120] : ["", 140]
twitter.update("#{@post.content.truncate(max)}#{img}")

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)
twitter.update(s)

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.

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

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"
      Twitter::Client.new.update(@post.to_tweet)
    end

    redirect_to root_path
  else
    @feed_items = []
    render 'static_pages/home'
  end
end
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Finbarr Apr 10 '13 at 2:37
  • \$\begingroup\$ can you show me what the method in the post model would look like? that would help me out a lot \$\endgroup\$ – user2159586 Apr 10 '13 at 2:37
  • \$\begingroup\$ @user2159586, updated my answer. \$\endgroup\$ – Mischa Apr 10 '13 at 2:41
  • \$\begingroup\$ I'd argue that posts shouldn't know about Twitter. \$\endgroup\$ – Dave Newton Apr 11 '13 at 16:19
2
\$\begingroup\$

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"

    @twitter = Twitter::Client.new

    if @post.image.present?
      @twitter.update(@post.content.truncate(120)+"... "+@post.image.path.to_s)
    else
      @twitter.update(@post.content.truncate(140))
    end
  end

  redirect_to root_path
end
\$\endgroup\$
  • \$\begingroup\$ Personally, I think this is easier to read than mine. It's still in the wrong place, but that's a separate issue. Everything related to twitter should be completely removed from the code that creates the message, allowing for easier testing and alternative implementations. \$\endgroup\$ – Dave Newton Apr 10 '13 at 2:45
  • \$\begingroup\$ Yup, guess you commented before I edited. \$\endgroup\$ – Amit Kumar Gupta Apr 10 '13 at 2:48
  • \$\begingroup\$ 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. \$\endgroup\$ – Mischa Apr 10 '13 at 3:05
  • \$\begingroup\$ 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. \$\endgroup\$ – Amit Kumar Gupta Apr 10 '13 at 3:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy