Topic: Calculating a Due Date

I am having some difficulty calculating a due date from a specified turnaround time.  The turnaround time is in business days and I've constructed a loop to try to calulate the final real days count.  The code is part of the Create method in my JobsController:

`def create    begin    @job = Job.new(params[:job])    @job.company_id = @job.contact.company_id # Back-assign the company association    # New problem code    @turn = @job.turnaround # Int value for business days of turnaround    @daycount = 0 # Int value for number of real days    @count = 0 # An integer counter    while @count < @turnaround # Will loop until count = turnaround      if cwday(@accepted+(@daycount)) ==(6) # Commercial work date of the accepted date plus the current daycount falls on sat        @daycount += 1 # Increment the daycount      elsif cwday(@accepted+(@daycount)) ==(7) # Commercial work date of the accepted date plus the current daycount falls on sun        @daycount += 1      else # Otherwise...        @daycount += 1 # Increment the daycount        @count += 1 # Increment the counter      end    end    @job.duedate = @job.created_at +(@daycount) # Add the final tally of daycount to the creation date    # End of problem code    if @job.save      flash[:notice] = 'Job was successfully created.'      redirect_to :action => 'list'    else      @companies = Company.find_all      @contacts = Contact.find_all      @employees = Employee.find_all      render :action => 'new'    end    # NEW STUFF, the Rescue    rescue      flash[:notice] = 'Job could not be saved.'      redirect_to :action => 'new'    end  end`

I'm not sure where things are going horribly wrong, but the job just doesn't save.  Leading suspects are my used of the cwdate() feature or the day additions.  Any help would be greatly appreciated.

Thanks!
-Jacob

Re: Calculating a Due Date

One word: Refactoring

I'm sorry this doesn't apply directly to your question or problem, but I think it will really help you. The above code could use some serious refactoring. The method is long and overly complicated. There are a slew of instance variables which should be either temporary variables or removed completely.

Refactoring the code may reveal the error. It is the art of improving code design and quality without changing the external behavior of it. I'd suggest getting this book on refactoring, it is still the best way to learn it:
http://www.amazon.com/gp/product/020148 … 70-6713438

As far as your problem, what do you mean by "the job just doesn't save"? Do you get an error message? Or does everything seem successful except nothing is in the database?

Railscasts - Free Ruby on Rails Screencasts

Re: Calculating a Due Date

Actually, this seems like a good spot to put a little refactoring exercise. I hope you don't mind, Muffin Man, if I clean up the code a little.

First off, some instance variables (they begin with an @ sign) are used where there should be temporary variables. I'll replace them with temporary variables and remove some unnecessary parenthesis and comments. I'll also remove the rescue statement as that seemed to be used for testing purposes.

`def create  @job = Job.new(params[:job])  @job.company_id = @job.contact.company_id # Back-assign the company association  turn = @job.turnaround # in business days  daycount = 0 # in real days  count = 0  while count < turn    if cwday(@accepted+daycount) == 6 # Saturday      daycount += 1    elsif cwday(@accepted+daycount) == 7 # Sunday      daycount += 1    else      daycount += 1      count += 1    end  end  @job.duedate = @job.created_at + daycount    if @job.save    flash[:notice] = 'Job was successfully created.'    redirect_to :action => 'list'  else    @companies = Company.find_all    @contacts = Contact.find_all    @employees = Employee.find_all    render :action => 'new'  endend`

Take a look at the result of that calculation, it is being placed into the @job.duedate attribute. Why save this value at all? It can be calculated already, so let's move it to a "duedate" method in the Job model.

The main thing keeping us from moving this calculation out of the controller is the reference of the @accepted instance variable. I'm just going to use created_at instead because I think it's what you need. here's the calculation moved into a "duedate" method for the Job model.

`class Job < ActiveRecord::Base  def duedate    daycount = 0 # in real days    count = 0    turn = turnaround    while count < turn # in business days      if cwday(created_at + daycount) == 6 # Saturday        daycount += 1      elsif cwday(created_at + daycount) == 7 # Sunday        daycount += 1      else        daycount += 1        count += 1      end    end    created_at + daycount  endend`

The "turn" temporary variable is unnecessary, so we can remove it and just use the "turnaround" method call.

`class Job < ActiveRecord::Base  def duedate    daycount = 0 # in real days    count = 0    while count < turnaround # in business days      if cwday(created_at + daycount) == 6 # Saturday        daycount += 1      elsif cwday(created_at + daycount) == 7 # Sunday        daycount += 1      else        daycount += 1        count += 1      end    end    created_at + daycount  endend`

Notice we are incrementing the "daycount" variable on each condition of that logic? The only thing that changes is if the "count" variable is incremented or not. Simplify the logic like this:
`class Job < ActiveRecord::Base  def duedate    daycount = 0 # in real days    count = 0    while count < turnaround # in business days      daycount += 1      count += 1 if cwday(created_at + daycount) <= 5    end    created_at + daycount  endend`

Now that I can understand the code I see a couple problems. I'm assuming "created_at" returns a Time object, so adding an integer to a Time results in adding seconds to the time, not days. Let's fix that by storing the seconds instead (using the Integer "day" method).
`class Job < ActiveRecord::Base  def duedate    seconds = 0    count = 0    while count < turnaround # in business days      seconds += 1.day      count += 1 if cwday(created_at + seconds) <= 5    end    created_at + seconds  endend`

The other problem is that "cwday" is a method of the Date class and should be called on an instance of that class, the method doesn't accept any arguments. We can use the Time "to_date" method to convert a time to a date
`class Job < ActiveRecord::Base  def duedate    seconds = 0 # in real days    count = 0    while count < turnaround # in business days      seconds += 1.day      count += 1 if (created_at + seconds).to_date.cwday <= 5    end    created_at + seconds  endend`

There is another refactoring we can do. That "created_at + seconds" calculation is duplicated. We can remove that by setting seconds to the "created_at" time to begin with. Let's rename the seconds variable to "time" while we do that.

`class Job < ActiveRecord::Base  def duedate    time = created_at    count = 0    while count < turnaround # in business days      time += 1.day      count += 1 if time.to_date.cwday <= 5    end    time  endend`

That "time.to_date.cwday <= 5" area isn't very self descriptive. We can refactor this part out into a "business_day?" Time method. That way, to see if the time falls on a business day, we just call it's "business_day?" method. I can see that method being useful elsewhere as well.

`class Job < ActiveRecord::Base  def duedate    time = created_at    count = 0    while count < turnaround # in business days      time += 1.day      count += 1 if time.business_day?    end    time  endendclass Time  def business_day?    self.to_date.cwday <= 5  endend`

That looks really good. We can even refactor this a bit further by making a "business_days_since" method for Integer.

`class Job < ActiveRecord::Base  def duedate    turnaround.business_days_since created_at  endendclass Integer  def business_days_since(time = Time.now)    business_days = self    while business_days > 0      time += 1.day      business_days -= 1 if time.business_day?    end    time  endendclass Time  def business_day?    self.to_date.cwday <= 5  endend`

That's it for the refactoring exercise. Pretty cool eh?

Have you noticed there are no comments which pertain to that code? Comments are often a sign that the code itself isn't clear. Next time, instead of adding a comment, try to make the code itself a little clearer through refactoring.

I see a couple of other refactorings you can do in the controller "create" method. I'll leave those as an exercise for the reader.

Last edited by ryanb (2006-07-11 02:30:38)

Railscasts - Free Ruby on Rails Screencasts

Re: Calculating a Due Date

I think that's a really good exercise in refactoring but you did lose me slightly with the last change and the introduction of business_days_since.

Of course we're not allowing for public holidays in this algorithm.  You'd then need to work through all the public holidays you observe, adding one to your end date for each one that falls between the start date and the end date.  However, I'm not aware of a Rails function that correctly returns all the public holidays in England.

Allen

Re: Calculating a Due Date

allen wrote:

I think that's a really good exercise in refactoring but you did lose me slightly with the last change and the introduction of business_days_since.

Yeah, that was kind of a final "throw in" if you really want to go all out. What I was trying to achieve is the readability you get from some of the rails date helper calls (i.e. 5.days.since(time)).

allen wrote:

Actually, I considered that at first but I don't think it will work. For example, let's say the turnaround is 8 business days and the job is created on a Thursday, the job would span across two weekends (four extra days). If the job is created on a Monday it would span only one weekend. Seeing if the creation falls on a weekend is not enough. Adding any further logic would be more complicated than the loop, so I think the loop is the best method.

allen wrote:

Of course we're not allowing for public holidays in this algorithm.  You'd then need to work through all the public holidays you observe, adding one to your end date for each one that falls between the start date and the end date.  However, I'm not aware of a Rails function that correctly returns all the public holidays in England.

Yeah, I probably wouldn't have gone to the extent of moving the logic into the Time and Integer classes if we want holidays because they are so specific to the circumstances.

Railscasts - Free Ruby on Rails Screencasts

Re: Calculating a Due Date

ryanb wrote:
allen wrote:

Actually, I considered that at first but I don't think it will work. For example, let's say the turnaround is 8 business days and the job is created on a Thursday, the job would span across two weekends (four extra days). If the job is created on a Monday it would span only one weekend. Seeing if the creation falls on a weekend is not enough. Adding any further logic would be more complicated than the loop, so I think the loop is the best method.

OK, let's try that.

Case 1: I'd break the eight days into a week and three days.  Adding the week takes me to the following Thursday, adding the three days takes me to the Sunday.  Then I apply the check about it being a Saturday or a Sunday and advance it to the Monday.

Case 2: I'd again break the eight days into a week and three days.  Adding the week takes me to the following Monday, adding the three days takes me to the Thursday.  The check about it being a Saturday or a Sunday fails so it stays at the Thursday.

QED!

Re: Calculating a Due Date

Ohh, I guess that will work, I was looking at it incorrectly. Thanks allen.

I don't know if the resulting logic would be less complicated than a loop, but it would definitely be more efficient.

Edit:
On second thought, what if the creation date is on a Thursday and the turnaround is 4 days? Adding 4 days will land you on a Monday which will not trigger the exception, but what you want is Wednesday. Am I still misunderstanding your solution?

Last edited by ryanb (2006-07-12 11:21:27)

Railscasts - Free Ruby on Rails Screencasts

Re: Calculating a Due Date

Thanks for the replies, guys.  I got the loop working and decided to leave it within create.  That way, I'll be able to easily add the logic later to account for a table of statutory holidays (user generated).

Re: Calculating a Due Date

ryanb wrote:

Edit:
On second thought, what if the creation date is on a Thursday and the turnaround is 4 days? Adding 4 days will land you on a Monday which will not trigger the exception, but what you want is Wednesday. Am I still misunderstanding your solution?

No you're not.  Back to the drawing board...