Rails guestbook with new, index, and Kaminari pagination in the same method

Question

I created a guestbook that on one page displays both the form for a new entry and all of the entries that have been created so far.

I am trying to use Kaminari for pagination for the entries.

Right now this code is working on my local machine, but I just have that feeling that it could be improved/refactored, so I wanted to post it here for people's opinions.

My controller:

class GuestbooksController < ApplicationController
    def index
        @guestbooks = Guestbook.all
    end

    def new
        @guestbook = Guestbook.new
        @guestbooks = Guestbook.all
        @guestbooks = Kaminari.paginate_array(@guestbooks).page(params[:page]).per(5)
    end

    def create
        @guestbook = Guestbook.new(guestbook_params)
        @guestbooks = Guestbook.all
        @guestbooks = Kaminari.paginate_array(@guestbooks).page(params[:page]).per(5)

        if @guestbook.save
            flash.now[:notice] = "Thanks for taking the time to write us! We greatly appreciate it!"
            render :new
        else
            flash.now[:notice] = "Your message failed to post, please try again"
            render :new
        end
    end

    private
    def guestbook_params
        params.require(:guestbook).permit(:name, :email, :message)
    end
end

new.html.erb template:

<div class="paginate">
    <%=paginate @guestbooks%>
</div>

<div class="span1">
    <% @guestbooks.each do |g| %>
        <br/>
        <h4><%= g.name %>, <%= g.created_at %><br/></h4>
        <%= g.message %><br/>
        <p>-----------------------------------------------------------------------------------------------------------------------</p>
    <% end %>
</div>

<%= simple_form_for @guestbook, url: guestbooks_path, method: :post do |f| %>
    <div class="span2"><%= f.input :name %></div>
    <div class="span2"><%= f.input :email %></div>
    <div class="span3"><%= f.input :message %></div>
    <div class="span4"><%= f.button :submit %></div><br/><br/><br/>
<% end %>

Routes:

 resources :guestbooks, only: [:new, :create]
  get "/guestbooks/index", to: "guestbooks#index"

Now I know my routes file could look like this

resources :guestbooks, only: [:new, :create, :index]

but then my paths look strange:

        guestbooks GET  /guestbooks(.:format)             guestbooks#index
                   POST /guestbooks(.:format)             guestbooks#create
     new_guestbook GET  /guestbooks/new(.:format)         guestbooks#new

Answer 1

Yes, there's a lot that can be done differently.

Firstly, Ruby convention is 2 spaces of indentation. Not 4, not tabs.

Secondly, Guestbook isn't a great name for your model. You're modelling entries in a guestbook, not multiple guestbooks.

Mostly, though I fail to understand the structure of your pages. If you go to index you load all the entries. If you go to new you... also load all the entries. But then you do some pagination, which, in effect, discards all but 5 of the records you just loaded from the database. Imagine how well that'll work if you have thousands of entries...

But why is the new action listing and paginating anything? That's the job of the index action; to list things, to be an index. Sure, you can combine the two by having a form and a list on the same page, but in that case it should be on the index view.

I'd cut the controller down to this (keeping, for now, the Guestbook model name):

class GuestbooksController < ApplicationController
  before_action :load_guestbooks

  def index
    @guestbook = Guestbook.new
  end

  def create
    @guestbook = Guestbook.new(guestbook_params)

    if @guestbook.save
      redirect_to guestbooks_url, notice: "Thanks for taking the time to write us! We greatly appreciate it!"
    else
      flash.now[:alert] = "Your message failed to post, please try again"
      render :index
    end
  end

  private

  def load_page
    @guestbooks = Guestbook.page(params[:page])
  end

  def guestbook_params
    params.require(:guestbook).permit(:name, :email, :message)
  end
end

No new action whatsoever. I haven't used kaminari, but just going to its github page I see several examples of usage that are much cleaner than loading everything. The whole point of pagination is not do that.

For your view (which should be the index view), I would read up on class naming, semantics, and HTML in general, I'm sorry to say. For instance, the hr tag could save you a few dozen hyphens, and span1, span2 etc. are not useful names for anything - certainly not divs. And don't use br for layout; use CSS.

Also, look into partials, aka partial views. If a partial is properly used, the whole entries list can be rendered as just:

<%= render @guestbooks %>

Lastly for the view, simple_form does not need a url or method argument if they can be derived from the record. Which they can in this case.

For your routes, you only need this:

resources :guestbooks, only: [:index, :create]

The paths in your question do not look strange: Two of them are both called guestbooks_path because they do indeed have the same path - but the action they invoke depend on the method (POST or GET) used. If you POST to /guestbooks it'll invoke the create action; if you simply GET /guestbooks, it'll invoke the index action. That's the basic premise of routing in Rails.

Answer 2

Not much to add from Flambino's answer. I wholeheartedly agree about the class names in the html. As a general rule you should do as much as possible in the CSS and as little as possible in the HTML. Consider using <section> tags and the like for more semantic html.

Using class names like 'span1', 'span2' etc is severely frowned on. With your masterful CSS, Ben, it should be no problem to use nth-childs to target the div you want. Give the containing div a class or id if necessary, but only if necessary.

Good call by Flambino on taking the new action out of the controller completely, particularly since the corresponding new view is not required (remember to delete the now redundant new.html.erb file you may have lurking in your folders somewhere).