I have a data structure like this which holds start/end dates:

----|-----------|-----------
Item| start     | end
----|-----------|-----------
1   |2017-05-12 | 2017-05-12
2   |2017-05-12 | 2017-05-13
3   |2017-05-13 | 2017-05-13
4   |2017-05-13 | 2017-05-14
5   |2017-05-14 | 2017-05-15
6   |2017-05-15 | 2017-05-16
7   |2017-05-16 | 2017-05-16 

I want to move the start date of the first item to 2017-01-01 and every item should follow that start date and must stay within its ranges. The result should look like the following:

----|-----------|-----------
Item| start     | end
----|-----------|-----------
1   |2017-01-01 | 2017-01-01
2   |2017-01-01 | 2017-01-02
3   |2017-01-02 | 2017-01-02
4   |2017-01-02 | 2017-01-03
5   |2017-01-03 | 2017-01-04
6   |2017-01-04 | 2017-01-05
7   |2017-01-05 | 2017-01-05

This is how I implemented the method:

// ActiveItems() method (although it's bad naming) retrieves all the currently active hotels/transfers etc from a booking.  
var items = booking.ActiveItems(); // each item has check-in and/or check-out dates, internals doesn't matter for this problem

var tempCurrentDate = newStartDate;
var previousCheckoutDate = newStartDate;

// go thru each item
for (var i = 0; i < items.Count; i++)
{
    var currentItem = items[i];
    // again, bad naming but internals doesn't really matter. method returns a check-in date (DateTime)
    var itemCheckinDate = currentItem.CheckInDate(); 
    var itemCheckoutDate = currentItem.CheckOutDate(); 

    // no of days between check-out and check-in
    // this difference later added to the new check-in date to get the new check-out
    var checkinToCheckoutDifference = (itemCheckoutDate - itemCheckinDate).Days; 
    if (i == 0)
    {
        // check-in equals to start date if the item is the first one in the list
        itemCheckinDate = tempCurrentDate;
    }
    else
    {
        var prevDayToCurrentCheckinDifference = (previousCheckoutDate - currentItem.CheckInDate()).Days; 
        itemCheckinDate = tempCurrentDate.AddDays(prevDayToCurrentCheckinDifference);
    }

    //SetCheckInDate() internals really doesn't matter here.
    currentItem.SetCheckInDate(itemCheckinDate);

    previousCheckoutDate = itemCheckoutDate;

    tempCurrentDate = itemCheckinDate.AddDays(checkinToCheckoutDifference);

    currentItem.SetCheckOutDate(tempCurrentDate); 
}

Even though this code works as expected, is there a better and efficient way of doing this?

  • 3
    Why can't you just substract a number of days from every date? I'm struggling to figure out what would contradict that – bushed Jan 23 '17 at 11:46
  • @Paparazzi FWIW my moniker is Vogel612 :/ Sorry for the confusion, my chromium was so friendly as to use a non-fixed-width font for displaying monospace, which led me to believe that the table was misaligned ... – Vogel612 Jan 23 '17 at 12:34
  • I don't understand what the code is trying to achieve. In the result set, do you really intent do have item #7 = 2017-05-05 - 2017-05-05 or is that a typo? – JanErikGunnar Jan 24 '17 at 8:39
  • @JanErikGunnar it is not a typo. This is a list of items in a booking. and that #7 is a transfer item (which falls within the same day) – Romesh D. Niriella Jan 24 '17 at 8:50
  • @bushed can you show me some code? I'm struggling to figure it out. – Romesh D. Niriella Jan 24 '17 at 8:51
up vote 3 down vote accepted

Some quick remarks:

  • Be consistent in naming. You mix both CheckOut and Checkout. Ditto for CheckIn vs Checkin.
  • Why are CheckInDate() and CheckOutDate() methods? They should be properties. (Or you should rename each so they start with "Get" -- but really, they should be properties.)
  • tempCurrentDate is not a good name for what it represents.
  • Why did you prefix itemCheckinDate and itemCheckoutDate with "item"? Why not simply use checkinDate and checkoutDate?
  • Comments should tell me why, not what. // no of days between check-out and check-in doesn't tell me anything the code doesn't.
  • Don't needlessly abbreviate: "prevDay" in prevDayToCurrentCheckinDifference is unclear.

Your flow is very hard to follow and seems needlessly complicated. I'd for instance leave SetCheckInDate and SetCheckOutDate to the very end instead of splitting them up. Make your flow easier to follow:

  1. store existing dates in local variables
  2. update local variables
  3. update booking

Or even simpler:

        var earliestCheckInDate = items.Min(x => x.CheckInDate());
        var offSet = (newStartDate - earliestCheckInDate).Days;

        foreach (var item in items)
        {
            var incorrectCheckInDate = item.CheckInDate();
            var incorrectCheckOutDate = item.CheckOutDate();

            item.SetCheckInDate(incorrectCheckInDate.AddDays(offSet));
            item.SetCheckOutDate(incorrectCheckOutDate.AddDays(offSet));
        }

The logic is simple: since the shift is always the same amount of days, I calculate it once (using the earliest available date) and then apply that offset on each day. You could perhaps even avoid using incorrectCheckInDate and incorrectCheckOutDate.

  • 1
    I think ActiveItems() should also be a property – JanErikGunnar Jan 24 '17 at 8:36
  • @JanErikGunnar Yes, true. Missed that one. – BCdotWEB Jan 24 '17 at 9:45
  • hi @BCdotWEB , thank you for your invaluable remarks. This is legacy code I'm working with so I am not able to make changes to the original code. Thus, a couple of extension methods which should be properties are getting the job done. AND., you're awesome because this simple, elegant piece of code gets the job done perfectly. Here's the result pastebin.com/hMU0uMFR . Could you please briefly explain the thinking behind your code? – Romesh D. Niriella Jan 24 '17 at 10:45
var items = booking.ActiveItems(); // each item has check-in and/or check-out dates

var offset = items[0].GetCheckInDate().Date - DateTime.Parse("2017-05-12");

foreach(var item in items)
{
   item.SetCheckInDate(item.GetCheckInDate() - offset);
   item.SetCheckOutDate(item.GetCheckOutDate() - offset);
}

That, however would not account for e.g. holidays, weekends, availability

And the only reason to do iterative adjustmens (as in original code) would be if SetDate would automatically make adjustments to holidays/weekends inside.

In this case original code lacks comments around that area explaining that SetDate might change the date by itself and absolute day offset would change.

  • Hi @bushed, this code doesn't really work as expected. here's the output. pastebin.com/yDpuKZ4L – Romesh D. Niriella Jan 24 '17 at 10:39
  • Not sure what's the problem pastebin.com/eX7uNreC – bushed Jan 24 '17 at 11:07
  • 2
    To be honest the whole point of my answer is to highlight that if that is some code that is being refactored - there could be consequences to any changes. It seems to me that code is taken out of context. And if my guess is right, as I mentioned in answer, the only wrong thing with the original code besides naming(mentioned in another answer) is lack of comments in the important place. You need to provide Item class implementation to provide enough context. – bushed Jan 24 '17 at 11:11

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.