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

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
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


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

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
render :index
end
end

private

@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.

• If the guest book fails to save, wouldn't you just want to render the create page again? Redirecting to the index page seems a little more destructive than is necessary. Dec 7 '15 at 20:56
• @GregBurghardt If it fails to save, it renders the index view (with the form etc.). It only redirects on success. Dec 7 '15 at 21:31
• Ah, gotcha. I've been staring at code too long. Dec 7 '15 at 21:32

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).