How to clean up this code?

OK, I’ve got my code working. As you will no doubt infer from my
questions below, I am completely new to web development. If clueless
n00b questions drive you crazy, then hit now and move on…

For those of you I haven’t scared away yet…
I have a table listing document names and numbers. I have a view with
a “select” box allowing the user to narrow the list of documents down
to those that fall into certain categories (as defined by the document
number prefix in the table). I display the table of documents using
active scaffold embedded in by index view.

The problem is… the code looks (to my n00b eyes) to be very
inelegant. I was wondering how others solve this problem or, how you
would clean up this code to make it look less kludgy…

Here is a snippet from my controller…

=======================================
class DocumentsController < ApplicationController
CATEGORIES =
[{:id => 0, :text => ‘All’, :min=>0, :max => 99},
{:id => 1, :text => ‘Standard Operating Procedures’, :min=>0, :max
=>0},
{:id => 2, :text => ‘Functional Group Policies and Procedures’,
:min => 1, :max => 1},
{:id => 3, :text => ‘Project Level Documentation’, :min => 2, :max
=> 9},
{:id => 4, :text => ‘Software Documentation’, :min => 10, :max =>
19},
]
CATEGORIES_FOR_SELECT = CATEGORIES.collect {|c| [c[:text], c[:id]]}

active_scaffold :document do |config|
# …
end

GET /documents

GET /documents.xml

def index
@documents = Document.find(:all)
@category = params[:category].to_i

respond_to do |format|
  format.html # index.html.erb
  format.xml  { render :xml => @documents }
end

end

def set_category
redirect_to documents_path(:category => params[:category])
end

end

=====================================
and here is a snippet from my view:

<%= content_tag :h1, “Document Log” %>
<% content_tag :fieldset do -%>
<% form_tag url_for_options = {:action => :set_category} do -%>
<%= content_tag :legend, “Document Category” %>
<%= select_tag ‘category’,
options_for_select(DocumentsController::CATEGORIES_FOR_SELECT,
@category) %>
<%= submit_tag ‘Go’ %>
<% end -%>
<% end -%>

<% @category ||= 0 %>
<% min = DocumentsController::CATEGORIES[@category][:min] %>
<% max = DocumentsController::CATEGORIES[@category][:max] %>

<%= render :active_scaffold => :documents, :conditions => "document_number_prefix >= #{min} AND document_number_prefix <= #{max}" %>
=========================================

To me, this looks a little kludgy (because it has been kludged
together 15 minutes at a time as I’ve been starting to learn RoR)

  1. Is this how folks typically code select boxes with constant
    content? Being a long time C programmer, I tend to think in terms of
    “struct”. So I created a class level constant array,
    DocumentsController::CATEGORIES containing :id, :text, :min, and :max
    fields. From that array of structures (see, my “C” is coming out), I
    derived a new class level constant array,
    DocumentsController::CATEGORES_FOR_SELECT containing just the :text
    and :id fields (in that order) which I could pass to
    #options_for_select.

  2. It seems odd to me to invoke the #set_category action just to have
    it redirect back to the index action. (This is my code review in
    action here – I’m looking at the code now and saying, gee that could
    probably have been made cleaner). Under RESTful routing, is there a
    way I could have had the submit button point right back at
    #documents_path with the :category parameter set?

  3. I think I am most dissatisfied with my view… It seems like a
    lot of ERB code and very little HTML. Is that typical? What is the
    more common practice in the Rails community – to use

<%= content_tag :h1, “Document Log” %>

or to use

Document Log

in the view?

  1. What about the rest of the ERB code? Did I use the #form_tag and
    #select_tag helper functions in the way that you (more experienced RoR
    developers) would use them, for this simple example of creating a
    select box of constant values?

  2. How about my ERB code that extracted “min” and “max” from
    DocumentsController::CATEGORIES… is that the way you would do it?

  3. I implemented this for now as a “select” tag with a “Go” button
    that results in a new URL with the category parameter tacked on…
    documents?category=1. Ultimately, I would like to learn enough about
    Ajax to update the view of the table dynamically and automatically
    when a user selects a new category. If anybody can give me a pointer
    on where to get started with that, I would appreciate it.

If you’ve read this far, thank you very much. I have trouble writing
short mailing list posts :slight_smile: If you happen to have opinions on and/or
answers to my questions and reply, thank you even more!

–wpd

Patrick D. wrote alot:

What I always find interesting is that different people can approach
essentially the same problem from very disparate approaches. But I’ll
weigh in with my two cents…

My categories would go in a table/model all their own. After all, they
are just another form of data for the application, and ideally, if you
needed to support another document type, it’s just an add of a data
record, and the application code stays the same. Suppose you need to add
a new doc type ‘QA Testing Protocols’? Do you alter your code, or add a
new record that looks like:

:id => 5, :text => ‘QA Testing Protocols’, :min => 20, :max => 29

Ajax: Haven’t gone there yet myself either.

I try diligently to keep code out of my views (with varying degrees of
success). Rather than a find(:all) in your controller, could you apply
the conditions there so the view just renders the documents it receives
(strict MVC adherents would say the view should only render what it is
given).

def index
if params[:category]
@category = params[:category].to_i
# assuming that new model for document categories
@doc_category = DocCategory.find_by_id(@category)
min = @doc_category.min.to_s
max = @doc_category.max.to_s
cond = [“document_number_prefix >= ? AND
document_number_prefix <= ?”, min, max]
else
cond = [“0=0”]
end
@documents = Document.find(:all, :conditions => cond)

respond_to do |format|
  format.html # index.html.erb
  format.xml  { render :xml => @documents }
end

end

then part of your view, at least, becomes:

<%= render :active_scaffold => :documents %>

Lastly, and this is strictly a personal preference, I very much like
haml for my views as opposed to erb. I got very tired of div, span, and
<%= %> spam in my views… Your mileage may vary.

On Fri, Mar 28, 2008 at 9:41 AM, Ar Chron
[email protected] wrote:

Patrick D. wrote alot:
Boy did that make me laugh! :slight_smile:

What I always find interesting is that different people can approach
essentially the same problem from very disparate approaches. But I’ll
weigh in with my two cents…
Thank you… that is precisely what I was hoping for!

My categories would go in a table/model all their own. After all, they
I’ve argued myself in and out of that position a couple of times.
Ultimately, I couldn’t see the difference between adding the line of
code that looked like:

:id => 5, :text => ‘QA Testing Protocols’, :min => 20, :max => 29

to my controller, vs adding an entry to a table in the database. But
if the more common practice is to put data like that in a model, then
I would like to follow the common practice.

I try diligently to keep code out of my views (with varying degrees of
success). Rather than a find(:all) in your controller, could you apply
the conditions there so the view just renders the documents it receives
(strict MVC adherents would say the view should only render what it is
given).

Thanks for the tips. As I looked at the code more carefully (of
course, after submitting my long boring post), I noticed the

@documents = Document.find(:all)

line leftover from the scaffolding. The way Active Scaffolding works,
you pass the name of the model, not list of data to be displayed, so
that line is superfluous. However, I now see how I could set my
“cond” condition as an instance variable and simply use that in my
view as the condition. Thanks… that cleans things up.

Lastly, and this is strictly a personal preference, I very much like
haml for my views as opposed to erb. I got very tired of div, span, and
<%= %> spam in my views… Your mileage may vary.
OK… now I’ll go find out what haml is… thanks for the pointer.

–wpd

Patrick D. wrote:

I’ve argued myself in and out of that position a couple of times.

But if the more common practice is to put data like that in a model, then
I would like to follow the common practice.

I don’t know if it is common practice or not, but the decision point for
me is that I “own” the application, and the users “own” the data. If I
can drive a portion of the my application from data they provide, I can
delegate maintenance of that data to the person who owns it.

If the document gods wanted 50 more document categorizations so we now
span 0…149, or suddenly decided that the old 10…19 group is really two
groups, 10…15, and 16…19 with new names, I’d rather provide them a
simple interface so they can maintain their own data than go in tweaking
my code.

After all, us developers want to do the cool stuff, and what is
essentially a data entry task is far from cool… :wink:

Well, I’ve managed to clean up the code to:

========================================
class DocumentsController < ApplicationController
CATEGORIES =
[{:id => 0, :text => ‘All’, :min=>0, :max => 99},
{:id => 1, :text => ‘Standard Operating Procedures’, :min=>0, :max
=>0},
{:id => 2, :text => ‘Functional Group Policies and Procedures’,
:min => 1, :max => 1},
{:id => 3, :text => ‘Project Level Documentation’, :min => 2, :max
=> 9},
{:id => 4, :text => ‘Software Documentation’, :min => 10, :max =>
19},
]
CATEGORIES_FOR_SELECT = CATEGORIES.collect {|c| [c[:text], c[:id]]}

active_scaffold :document do |config|

end

GET /documents

GET /documents.xml

def index
if params[:category]
@category = params[:category].to_i
min = CATEGORIES[@category][:min]
max = CATEGORIES[@category][:max]
@condition = “document_number_prefix >= #{min} AND
document_number_prefix <= #{max}”
end
@CATEGORIES = CATEGORIES_FOR_SELECT
respond_to do |format|
format.html # index.html.erb
format.xml { render :xml => @documents }
end
end
end

for my controller, and, for my template:

=====================================
<%= content_tag :h1, “Document Log” %>
<% content_tag :fieldset do -%>
<% form_tag({:action => :index}, {:method => :get}) do -%>
<%= content_tag :legend, “Document Category” %>
<%= select_tag ‘category’, options_for_select(@CATEGORIES,
@category) %>
<%= submit_tag ‘Go’, :name => nil %>
<% end -%>
<% end -%>

<%= render :active_scaffold => :documents, :conditions => @condition %>
======================================

and I’m happier with this… While I don’t know the future, it’s not
likely that the categories will change during the lifetime of the
code. There are other bits and pieces of code that are likely to be
dependent upon this particular document categorization strategy. And,
while I could be accused of premature optimization, I like the idea of
passing a constant array to my view instead of invoking a database
query to display constant information.

Now I need to go learn about haml and see if that makes my view of the
template happier.

Thanks again for your insights.

–wpd