Try putting your result in an entirely different div, this way you won't need to write any logic.
Darren Evans 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: <div id="form_new"> <table border="1" cellpadding="0" cellspacing ="0"> <tr> <td> <%= 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%> </select>
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.
<%if !(@districts.empty?)%> <br> <select id="form2" name="form2" onchange="this.form.onsubmit(); return false;"> <%= options_from_collection_for_select @districts, "id", "district_name", @district_id.to_i%> </select> <%end%>
<%if (@institutions)%> <%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. <br/>. 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%> </select> <%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 </form>. 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> </tr> </table> </div>
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?
@county_id=Countyname.find(:first, :conditions => ["county_id = ?", params[:form1].to_i]).id end
@districts=District.find(:all, :order => "district_name", :conditions=>["county_id = ?", @county_id]) if request.post? @dist=District.find(:first, :conditions => ["id = ? and county_id = ?", params[:form2].to_i, @county_id]) if (@dist) @district_id =@dist.id 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 @.
if (params[:form1] && params[:form2]) @places=Place.find(:all, :order => "institution_id", :conditions => ["district_id = ?", @district_id]) @institutions= @institution_display=1 for place in @places @res=Institution.find(:first, :conditions=>["id = ?", place.id]) @institutions << @res if request.post? @institution_id=params[:form3] 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.
if !(@institution_id.nil?) && (@district_id && @county_id && !@institution_id.empty?) session[:institution_id]=@institution_id session[:district_id]=@district_id session[:county_id]=@county_id session[:institution_name]=Institution.find(:first, :conditions => ["id= ?", session[:institution_id]]).name session[:district_name]=District.find(:first, :conditions => ["id= ?", session[:district_id]]).district_name session[:county_name]=Countyname.find(:first, :conditions => ["id= ?", session[:county_id]]).county_name #render(:text => "<script>window.location.href = 'http://localhost:3000/login/register’</script>")
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 Development 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.