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. smile

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'
  end
end

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
  end
end

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
  end
end

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
  end
end

Now that I can understand the code wink 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
  end
end

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
  end
end

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
  end
end

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
  end
end

class Time
  def business_day?
    self.to_date.cwday <= 5
  end
end


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
  end
end

class 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
  end
end

class Time
  def business_day?
    self.to_date.cwday <= 5
  end
end


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

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.

My approach would be slightly different and I'd start by trying to re-engineer what the function is trying to achieve.  If it's simply adding X business days to a date then that's simple and doesn't require a loop.  There are (using TheMuffinMan's definition) always 5 business days in every seven so adding, say, 10 business days just means adding 2 weeks.  Adding 11 business days means adding 2 weeks and then an extra day.  So, instead of a loop, I'd divide by 5, add that (integer) number of weeks and then add the remainder.  Only then would I look to see if I'd landed on a Saturday or Sunday and move on to the Monday if I had.

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:

My approach would be slightly different and I'd start by trying to re-engineer what the function is trying to achieve.  If it's simply adding X business days to a date then that's simple and doesn't require a loop.  There are (using TheMuffinMan's definition) always 5 business days in every seven so adding, say, 10 business days just means adding 2 weeks.  Adding 11 business days means adding 2 weeks and then an extra day.  So, instead of a loop, I'd divide by 5, add that (integer) number of weeks and then add the remainder.  Only then would I look to see if I'd landed on a Saturday or Sunday and move on to the Monday if I had.

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:

My approach would be slightly different and I'd start by trying to re-engineer what the function is trying to achieve.  If it's simply adding X business days to a date then that's simple and doesn't require a loop.  There are (using TheMuffinMan's definition) always 5 business days in every seven so adding, say, 10 business days just means adding 2 weeks.  Adding 11 business days means adding 2 weeks and then an extra day.  So, instead of a loop, I'd divide by 5, add that (integer) number of weeks and then add the remainder.  Only then would I look to see if I'd landed on a Saturday or Sunday and move on to the Monday if I had.

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. smile

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...