Darren E. wrote:
Thanks for the reply. That doesn’t work though (I changed it to form_new
to test), maybe I am doing something stupid?
One code review coming up.
Darren
View:
|
<%= form_remote_tag :update => "form_new", :url => {:action =>
"form" }, :complete => evaluate_remote_response%>
I never use :update any more. I always use RJS in the action to point
to the specific variables that need an update.
Further, I think that updating the entire DIV containing a FORM is bad
luck. Of course it will work, but I would rather the browser can
preserve the original FORM’s context, at least so the user doesn’t
lose their focus and selection at submit time.
You are allow to update things other than DIVs, you know!
<select id="form1" name="form1"
onchange=“this.form.onsubmit();”>
<%= options_from_collection_for_select @counties, “id”,
“county_name”, @county_id.to_i%>
That will submit the FORM each time the user changes a value. This is
bad usability, and it would repaint the FORM, destroying the user’s
context before they can select the value they actually want. Submit
buttons are sometimes good things; they help the user decide and then
commit in two steps.
<%for institution in @institutions%>
<%institution.name%>
That has to mean <%= institution.name %>, right?
<%end%>
I had forgotten Ruby supports a for command. You should practice using
@institutions.each do, because it provides tighter scope, and leads to
more elaborate iterations, such as .insert, .reject, .map(&:name),
etc.
<br>
I always write XHTML.
. And my test cases use assert_xpath which
demand XHTML (for short segments of the HTML.)
You do have test cases, don’t you?
<select id="form3" name="form3"
onchange=“this.form.onsubmit(); return false;”>
<%= options_from_collection_for_select @institutions,
“id”, “name”, @institution_id.to_i%>
<%end%>
<%= submit_tag “Go” %>
Why you have submit tags built into the select items, and a submit
button? Those onchange handlers might not do what you think.
<%= end_form_tag %>
That thing is deprecated. I suspect one should use . That leads
to the lamentable situation where a <%%> starts a block and a </> ends
it, but I suspect the form_ tags also take do…end blocks, so <% end
%> might work there if you put a do up top.
</td>
|
Controller:
This action is way too long. You should write tests for all of it, and
then refactor it to pull all the model-specific stuff out to the
model, or to helpers. For example…
def form
@counties=Countyname.find(:all, :order =>“county_name”,
:conditions=>[“language_id = ?”, session[:language_id]])
That could be @counties = county_list(), where a helper hides the ugly
stuff and gives it a useful name.
if (params[:username])
session[:user_suggested]=(params[:username])
session[:password_suggested]=(params[:password])
redirect_to(:controller => ‘login’, :action => ‘login’)
return
end
You have several little actions here masquerading as one big action.
Split them up into multiple actions, give each one a name and a guard
clause enforcing how the browser must call it, and make each action as
short and sweet as possible.
The guard clause for that username action could be
return if request.xhr?
for example, to prevent Ajax from calling it (if that’s what you mean).
And the Joy of Ruby is how close you can get these expressions to
normal English. Enforce that wherever you can, including in your
variable name selections. Upgrade username to user_name.
if request.post?
Again, this is another action hiding inside this one. its guard clause
would be return unless request.post?
@district_id [email protected]
else
@district_id=District.find(:first, :conditions => [“county_id =
?”, @county_id]).id
end
end
I don’t think every single one of your variables needs a @.
end
end
I suspect this would work, off the top of my head:
institutions = Institution.find(places.map(&:id))
There is some variation on find() that takes an array of ids, so don’t
call find() in a loop if you can just pass that array in.
?", session[:county_id]]).county_name
#render(:text => “”)
I never use the session object, and you have a “latent model” here.
You should pull all those variables out of the session and put them
into a model, with a good name that reveals its intent.
render :update do |page|
page.redirect_to(:controller => ‘login’, :action => ‘register’)
end
render :update is incompatible with the :update option in the
form_remote_tag.
Next, you are redirecting to paint a complete page, with its layout
and everything. Rails doesn’t care that you are pushing the result
into a DIV in another page!
end
end
end
Try moving all the DIV’s contents into a partial, with a name better
than _form.rhtml, and then <% render :partial => ‘form’ %> inside the
calling RHTML.
Then leave in the :update tag, for now, and replace this render with
render :partial => ‘form’
After that works, look up the system to pass variables into a partial,
so you can take out lots of session and @.
The reason we all like Rails here is we have experience with Brand X,
where all forms and actions are completely crammed with sickeningly
irrelevant details. Rails leads us to write forms and actions that
really do look like the ones in the Agile Web D. with Rails
books - very short, completely readable, with almost no administrivia
and paperwork just to get something done. You should spend a little
time learning more Ruby, and then seeing how small you can make all
this code, before adding more features to it.
–
Phlip
http://c2.com/cgi/wiki?ZeekLand ← NOT a blog!!