# How to clean up this code?

**URL:** <https://www.ruby-forum.com/t/how-to-clean-up-this-code/133170>\
**Category:** Rails\
**Created:** [March 28, 2008, 1:55pm UTC](https://www.ruby-forum.com/t/how-to-clean-up-this-code/133170 "2008-03-28T13:55:03Z")\
**Posts on this page:** 5\
**Page:** 1

<div class="post-metadata">

**Author:** ![oldbug](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/o/c0e974/32.png) [@oldbug](https://www.ruby-forum.com/u/oldbug)\
**Post date:** [March 28, 2008, 1:55pm UTC](https://www.ruby-forum.com/t/how-to-clean-up-this-code/133170/1 "2008-03-28T13:55:03Z")

</div>

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 🙂 If you happen to have opinions on and/or  
answers to my questions and reply, thank you even more!

–wpd

---

<div class="post-metadata">

**Author:** ![oldbug](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/o/c0e974/32.png) [@oldbug](https://www.ruby-forum.com/u/oldbug)\
**Post date:** [March 28, 2008, 2:41pm UTC](https://www.ruby-forum.com/t/how-to-clean-up-this-code/133170/2 "2008-03-28T14:41:45Z")

</div>

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.

---

<div class="post-metadata">

**Author:** ![oldbug](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/o/c0e974/32.png) [@oldbug](https://www.ruby-forum.com/u/oldbug)\
**Post date:** [March 28, 2008, 3:09pm UTC](https://www.ruby-forum.com/t/how-to-clean-up-this-code/133170/3 "2008-03-28T15:09:48Z")

</div>

On Fri, Mar 28, 2008 at 9:41 AM, Ar Chron  
[removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) wrote:

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

> 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

---

<div class="post-metadata">

**Author:** ![oldbug](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/o/c0e974/32.png) [@oldbug](https://www.ruby-forum.com/u/oldbug)\
**Post date:** [March 28, 2008, 4:56pm UTC](https://www.ruby-forum.com/t/how-to-clean-up-this-code/133170/4 "2008-03-28T16:56:38Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![oldbug](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/o/c0e974/32.png) [@oldbug](https://www.ruby-forum.com/u/oldbug)\
**Post date:** [March 28, 2008, 8:34pm UTC](https://www.ruby-forum.com/t/how-to-clean-up-this-code/133170/5 "2008-03-28T20:34:42Z")

</div>

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