On 1 September 2010 17:59, Charlie B99 [email protected] wrote:
Hi all -
Ruby 1.8.7 / Rails 2.3.5
I have not worked right through your code but have got some comments
that may help (or not).
I want to list all requests pending my approval, along with the
remaining seats available.
What is a seat? This is the first time you have mentioned it.
def listPendingRequest
I would advise using the rails convention for naming things, so this
would be list_pending_requests
@allRequests = Request.find(:all, :conditions => [“state_id = ?”,
2]).each{
|request|
@schedule = Schedule.find(:first, :conditions => [“id =
?”,request.schedule_id])
If the above is finding the schedule for request you don’t need to do
a find, just use
@schedule = request.schedule.
@num = Request.find(:all, :conditions => [“state_id = ? AND schedule_id
= ?”, ‘3’, @schedule.id]).nitems
Now that you have a schedule, the requests for that schedule are
@schedule.requests, so you can do something like
@num = @schedule.requests.find(:all, “state_id = ?”, 3).count
Unless you need @num to be an instance variable of whatever class this
ends up in then just make it a simple variable - num
Is there a reason why you are using nitems rather than count?
@remaining = @schedule.capacity - @num
}
You have now dropped out of the block. Note that you overwrote
@remaining each time round. so it will now have the value of the last
time round. I am not sure that the block contents have achieved
anything at all.
@request = Array.new
If @request is going to contain a number of items call it @requests.
@allRequests.each do |r|
if (r.state_id == 2 and r.approver_id == current_user.id)
I am not sure what is going on here as I am running out of time. You
have not told us the relationships between Request and User. If you
find yourself using something.id then very often you just need to
setup the relationships in the right way and you can do the same sort
of thing as with schedules and requests above. So you may need
something like User has_many approved_requests with approver_id as the
foreign key, then User.approved_requests will give you the requests
that user has approved.
belongs in a model. I don’t know which model, though.
I suspect that it may be User method as it returns his pending
requests, but maybe not. Also consider whether this is really the
functionality you want. Often when the return from a method is
defined as returns something along with something else (as you have
done here) you actually want two separate methods. So maybe one that
gives the users unnaproved requests (which would be a User method
probably, and may have involve no code if you get the relationships
right) and a separate one to return remaining seats available
(whatever that means).
Also, this method shows the remaining seats for the first schedule on
all of the requests, regardless of which schedule is being requested.
When I do a @schedule = Schedule.find(:all…), I get an error:
“undefined method ‘capacity’ for Array”.
Does that only happen in the above code or do you mean that you just
cannot do Schedule.find(:all)? If you can do a simple find all then
gradually add bits to the working one till you get the problem. How
can the method be showing anything if you get that error?
Sorry I have probably not been able to give this the thought it
deserves but maybe this will at least be of some help.
Colin
