Making some discourse code a little better

I'm sure by this point everyone has seen Discourse, which is a new stab at forum and communication software. Indeed, it certainly seems well-polished and will certainly make a splash.

I've noticed that some of the code in the codebase is less than steller. I'm going to blog about refactoring a particularly nasty piece of their codebase, and dictate what I am learning along the way.

So first, the offender:

class TopicsController  
  def show
    @topic_view = TopicView.new(params[:id] || params[:topic_id],
                                current_user,
                                username_filters: params[:username_filters],
                                best_of: params[:best_of],
                                page: params[:page])

    # Consider the user for a promotion if they're new
    if current_user.present?
      Promotion.new(current_user).review if current_user.trust_level == TrustLevel.Levels[:new]
    end

    @topic_view = TopicView.new(params[:id] || params[:topic_id],
                                current_user,
                                username_filters: params[:username_filters],
                                best_of: params[:best_of],
                                page: params[:page])

    anonymous_etag(@topic_view.topic) do
      # force the correct slug
      if params[:slug] && @topic_view.topic.slug != params[:slug]
        fullpath = request.fullpath

        split = fullpath.split('/')
        split[2] = @topic_view.topic.slug

        redirect_to split.join('/'), status: 301
        return
      end
      # Figure out what we're filter on
      if params[:post_number].present?
        # Get posts near a post        @topic_view.filter_posts_near(params[:post_number].to_i)
      elsif params[:posts_before].present?
        @topic_view.filter_posts_before(params[:posts_before].to_i)      elsif params[:posts_after].present?
        @topic_view.filter_posts_after(params[:posts_after].to_i)
      else
        # No filter? Consider it a paged view, default to page 0 which is the first segment
        @topic_view.filter_posts_paged(params[:page].to_i)
      end
      View.create_for(@topic_view.topic, request.remote_ip, current_user)

      @topic_view.draft_key = @topic_view.topic.draft_key
      @topic_view.draft_sequence = DraftSequence.current(current_user, @topic_view.draft_key)

      if (!request.xhr? || params[:track_visit]) && current_user
        TopicUser.track_visit! @topic_view.topic, current_user
        @topic_view.draft = Draft.get(current_user, @topic_view.draft_key, @topic_view.draft_sequence)
      end

      topic_view_serializer = TopicViewSerializer.new(@topic_view, scope: guardian, root: false)

      respond_to do |format|
        format.html do
          @canonical = "#{request.protocol}#{request.host_with_port}" + @topic_view.topic.relative_url

          if params[:post_number]
            @post = @topic_view.posts.select{|p| p.post_number == params[:post_number].to_i}.first
            page = ((params[:post_number].to_i - 1) / SiteSetting.posts_per_page) + 1
            @canonical << "?page=#{page}" if page > 1
          else
            @canonical << "?page=#{params[:page]}" if params[:page] && params[:page].to_i > 1
          end

          last_post = @topic_view.posts[-1]
          if last_post.present? and (@topic_view.topic.highest_post_number > last_post.post_number)
            @next_page = (@topic_view.posts[0].post_number / SiteSetting.posts_per_page) + 2
          end

          store_preloaded("topic_#{@topic_view.topic.id}", MultiJson.dump(topic_view_serializer))
        end

        format.json do
          render_json_dump(topic_view_serializer)
        end
      end
    end

  end
end  

Whoa momma! That is a hairy function. Indeed, the TopicsController is the worst offender in terms of complexity in the entire codebase! Let's try to make the world a nicer place here.

One of the nice things about the Discourse codebase is the even though there are a few hairy parts of the app, the app itself is very well tested, and indeed the code coverage on TopicsController#show is nearly 100%.

Let's get started.

Beginning to refactor

The first item of the show function is some logic that considers whether the user should be considered for a promotion.

# Consider the user for a promotion if they're new
if current_user.present?  
  Promotion.new(current_user).review if current_user.trust_level == TrustLevel.Levels[:new]
end  

This code can move into a before_filter, since it doesn't really have anything to do with showing a post.

before_filter :consider_user_for_promotion, only: :show

def consider_user_for_promotion  
  return unless current_user.present?
  Promotion.new(current_user).review if current_user.trust_level == TrustLevel.Levels[:new]
end  

But then it occured to me that we're actually doing some pretty in-depth logic in order to run the promotion code. This piece of code

    if current_user.trust_level == TrustLevel.Levels[:new]

...is probably too much knowledge for the TopicsController. Indeed it seems that the Promotion class seems to be in flux, and that particular piece of logic is not necessary at the moment. By removing that logic we get to the final form for that method:

    def consider_user_for_promotion
      Promotion.new(current_user).review if current_user.present?
    end

Ah, much nicer.

Breaking down the components of the function

The rest of the function can be broken down in to 5 pieces:

  1. Initializing a TopicView, which is in charge of figuring out which topics to display,
  2. Doing some logic to enforce that a topic's slug matches,
  3. Doing tons of if gateways to determine which functions in TopicView to run,
  4. Tracking the visit of a particular user, and finally
  5. Running the logic to present the html or json to the client.

Slug logic

The next logical piece I sought to remove was the interesting logic around ensuring that the requested slug matches the topic id.

# force the correct slug
if params[:slug] && @topic_view.topic.slug != params[:slug]  
  fullpath = request.fullpath

  split = fullpath.split('/')
  split[2] = @topic_view.topic.slug

  redirect_to split.join('/'), status: 301
  return
end  

For the moment, I decided just to make them functions in the controller, with verbose names. Perhaps later we'll move them out of the controller, but for now simply moving
this logic out of the method is good enough.

def slugs_do_not_match  
  params[:slug] && @topic_view.topic.slug != params[:slug]
end

def redirect_to_correct_topic  
  fullpath = request.fullpath

  split = fullpath.split('/')
  split[2] = @topic_view.topic.slug

  redirect_to split.join('/'), status: 301
end  

...and in the controller, we end up with just a single line:

    redirect_to_correct_topic and return if slugs_do_not_match

...which reads much more easily.

Crazy gateways

The next piece of code is a series of if gateways that expose how tightly coupled the TopicsController and TopicView classes are. Essentially we are doing a lot of
work to determine which logic in TopicView to run.

# Figure out what we're filter on
if params[:post_number].present?  
  # Get posts near a post
  @topic_view.filter_posts_near(params[:post_number].to_i)
elsif params[:posts_before].present?  
  @topic_view.filter_posts_before(params[:posts_before].to_i)
elsif params[:posts_after].present?  
  @topic_view.filter_posts_after(params[:posts_after].to_i)
else  
  # No filter? Consider it a paged view, default to page 0 which is the first segment
  @topic_view.filter_posts_paged(params[:page].to_i)
end  

There is a lot of feature envy going on in the above example.

We are probably directing the logic of TopicView a little too much here. Figuring out which filter to run is not really the job of our controller.
The TopicView itself, since it knows how to filter, should understand how to filter itself.

So I started with writing just the interface I would like to see:

    @topic_view.filter_posts(post_number: params[:post_number],
                             posts_before: params[:posts_before],
                             posts_after: params[:posts_after])

Then I implemented that in lib/topic_view.rb, by simply removing that logic from the controller and pasting it into the TopicView class as a new method:

def filter_posts(opts = {})  
  if opts[:post_number].present?
    # Get posts near a post
    filter_posts_near(opts[:post_number].to_i)
  elsif opts[:posts_before].present?
    filter_posts_before(opts[:posts_before].to_i)
  elsif opts[:posts_after].present?
    filter_posts_after(opts[:posts_after].to_i)
  else
    # No filter? Consider it a paged view, default to page 0 which is the first segment
    filter_posts_paged(opts[:page].to_i)
  end
end  

Granted, that function is still a little crazy. But right now I am only concentrating on moving logic into better positions for readability and maintainability.

Here is a gist of the TopicsController as it stands currently.

There is even more we can put into the TopicView. We are initializing TopicView with a ton of parameters, and then calling filter_posts
later down the line. Why not have TopicView take care of all of that for me?

@topic_view = TopicView.new(params[:id] || params[:topic_id],
                            current_user,
                            username_filters: params[:username_filters],
                            best_of: params[:best_of],
                            page: params[:page],
                            post_number: params[:post_number],
                            posts_before: params[:posts_before],
                            posts_after: params[:posts_after])

This initialize method of TopicView is starting to get out of hand. Because there is a direct correlation between the names and values of the options, I thought it
might be okay if we just pass in params directly:

@topic_view = TopicView.new(params[:id] || params[:topic_id], current_user, params)

..which is now a one-liner.

The next 2 lines that seem kind of suspect are the following in TopicsController:

@topic_view.draft_key = @topic_view.topic.draft_key
@topic_view.draft_sequence = DraftSequence.current(current_user, @topic_view.draft_key)

@topic_view.topic.draft_key is a clear Demeter violation, and there is just no sense in having the controller handle this internal logic. It is another example of the controller dictating the internal state of a totally separate class. So those lines go into TopicView's initialize function:

class TopicView  
  def initialize(topic_id, user=nil, options={})

    ...

    @draft_key = @topic.draft_key
    @draft_sequence = DraftSequence.current(user, @draft_key)
  end
end  

Finally the last piece here is to deal with some logic that appears to be tracking the user, when the user clicks on a topic:

if (!request.xhr? || params[:track_visit]) && current_user  
  TopicUser.track_visit! @topic_view.topic, current_user
  @topic_view.draft = Draft.get(current_user, @topic_view.draft_key, @topic_view.draft_sequence)
end  

For now, I just extracted this stuff into new functions that live in the controller, and made the tracking a one-liner:

track_visit_to_topic  

See where we are at with the TopicsController refactor

Handling the responding logic

The html response seems to be creating a bunch of variables that will be used in the app/views/topics/show.html.erb template.

The first thing that jumped out at me is that there were a bunch of if gateways to determine how to set the @canonical variable. As it turns out, however, nearly all of
the variables needed for the view can be satisfied by a simple refactoring of the @topic_view instance, which is what I went ahead doing.

This means we can remove the entire section about setting the @canonical variable.

I added this in lib/topic_view.rb:

def canonical_path  
  path = @topic.relative_url
  path << if @post_number
    page = ((@post_number.to_i - 1) / SiteSetting.posts_per_page) + 1
    (page > 1) ? "?page=#{page}" : ""
  else
    (@page && @page.to_i > 1) ? "?page=#{@page}" : ""
  end
  path
end  

..and refactored the view to use @topic_view:

<%- content_for :canonical do %>  
  <%= "#{request.protocol}#{request.host_with_port}#{@topic_view.get_canonical_path} %>
<%- end %>  

(Granted there is probably a better way to do the request.protocol etc.)

The last bits of logic in the controller determine the value of @next_page, which will provide a link to the user, if there is another page of posts:

last_post = @topic_view.posts[-1]  
if last_post.present? and (@topic_view.topic.highest_post_number > last_post.post_number)  
  @next_page = (@topic_view.posts[0].post_number / SiteSetting.posts_per_page) + 2
end  

The above is yet another example of feature envy, and is rife with demeter violations, so the correct place for this code is in TopicView:

class TopicView  
  def next_page
    last_post = @posts[-1]
    if last_post.present? and (@topic.highest_post_number > last_post.post_number)
      (@posts[0].post_number / SiteSetting.posts_per_page) + 2
    end
  end

  def next_page_path
    "#{@topic.relative_url}?page=#{next_page}"
  end
end  

At the end of the day, the final TopicsController#show method looks something like this:

def show  
  @topic_view = TopicView.new(params[:id] || params[:topic_id], current_user, params)
  anonymous_etag(@topic_view.topic) do
    redirect_to_correct_topic and return if slugs_do_not_match
    View.create_for(@topic_view.topic, request.remote_ip, current_user)
    track_visit_to_topic
    perform_show_response
  end
end  

..which is much easier to read and maintain.

Conclusion

I really love how discourse is a solid contender to compete against the old guard LAMP stacks of PhpBB, invasion powerboard, bbpress, etc. The technology stack is much
better, and it seems just like a totally fresh start. I'm excited to contribute more and make Discourse a shining example of well-architected code.

Keep track of the pull request here!