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

Patrick Doyle 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:

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

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.

Patrick Doyle 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 Doyle 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: