I have the following method which should work in Rails, but I am not sure if this is the best implementation. It looks a little clumsy, given all the good helpers in rails. Am I missing a much better way to do this?
def get_months_and_years(st,ed) m = [st.month] d = st.beginning_of_month while d <= ed.beginning_of_month m << [date.month, date.year] d = d.next_month end return m end
Thanks,
Tim
No doubt there is a slicker way, but I suggest that maybe the thing that is a little clumsy is code that needs this functionality in the first place ( I mean building the array of numbers ). Can you not just keep the start and end dates and determine the month and year values at the time you need them (if you really need them at all that is).
Colin
I would do it this way:
def get_months_and_year(st, ed) m = if ed >= st years = ed.year - st.year if years == 0 (st.month…ed.month).each{|month| m << [month, st.year] } elsif years > 0 (st.month…12).each{|month| m << [month, st.year] } (st.year+1…ed.year-1).each{|year| (1…12).each{|month| m << [month, year] } } (1…ed.month).each{|month| m << [month, ed.year] } end end return m end
Just for not processing date objects many times. This code would be refactored.
Daniel Alejandro Gaytán Valencia
Colin,
Good point, but I am not sure of another way. I need to display calendars that show all events in a group. I'll look at the bigger picture again.
Best,
tim
Colin Law wrote:
Not sure exactly what you are doing of course, but could you write view helper code that takes the start and end dates and generates the appropriate view stuff?
Colin
As a couple of people have said, looking at this in isolation won't allow us to easily give suggestions of better solutions, but you could certainly tidy that method a little.
Firstly, (personal preference time!) your variable names aren't helping your code be "self documenting". Even in such a small method, it's frustrating to try to remember what all those single-character variables are supposed to be doing. If they're named to refer to what they do, they will help other people (or you when you come back to it in a couple of months!) understand what the intention behind their existence is.
Every time you loop round, you calculate "st.beginning_of_month" - it would be a bit more efficient to store the value for use in the comparison each time. You never use "st" again, so I'd just overwrite it.
The first element you populate the array with only has "st.month", all the others have the year too - not sure if this is a typo or intended? Another potential typo is in the array push; you refer to a "date" variable which I can't see set anywhere. I assume this is actually your "d" variable?
So, if I were writing your method to be clearer, it might look like this:
def get_months_and_years(start_date, end_date) end_date = end_date.beginning_of_month return_value = [start_date.month]
working_date = start_date.beginning_of_month while working_date <= end_date return_value << [working_date.month, working_date.year] working_date = working_date.next_month end return return_value end
But.... My inclination would be to be a bit more "Ruby"... assuming you want an array of arrays of month and year for every month between two dates:
def get_months_and_years(start_date, end_date) # it would be worth checking that the parameters are both valid dates/times (start_date..end_date).collect { date| [date.month, date.year] }.uniq end
In that method I'm just taking the month and year for every date in the range, and then returning unique values from the collection (so one for each month/year combination. I've not got any time to look into it, but I wonder if the Range.collect can step through month-at-a-time rather than day-at-a-time...
I don't know if it's any quicker or much slower, but it's certainly
more concise, and (to my eye) a bit clearer in its intention, and
looks a lot less like PHP 
HTH Michael