3
\$\begingroup\$

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?

\$\endgroup\$
8
  • 3
    \$\begingroup\$ Why can't you just substract a number of days from every date? I'm struggling to figure out what would contradict that \$\endgroup\$
    – bushed
    Jan 23, 2017 at 11:46
  • \$\begingroup\$ @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 ... \$\endgroup\$
    – Vogel612
    Jan 23, 2017 at 12:34
  • \$\begingroup\$ 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? \$\endgroup\$ Jan 24, 2017 at 8:39
  • \$\begingroup\$ @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) \$\endgroup\$ Jan 24, 2017 at 8:50
  • \$\begingroup\$ @bushed can you show me some code? I'm struggling to figure it out. \$\endgroup\$ Jan 24, 2017 at 8:51

2 Answers 2

3
\$\begingroup\$

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I think ActiveItems() should also be a property \$\endgroup\$ Jan 24, 2017 at 8:36
  • \$\begingroup\$ @JanErikGunnar Yes, true. Missed that one. \$\endgroup\$
    – BCdotWEB
    Jan 24, 2017 at 9:45
  • \$\begingroup\$ 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? \$\endgroup\$ Jan 24, 2017 at 10:45
3
\$\begingroup\$
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.

\$\endgroup\$
3
  • \$\begingroup\$ Hi @bushed, this code doesn't really work as expected. here's the output. pastebin.com/yDpuKZ4L \$\endgroup\$ Jan 24, 2017 at 10:39
  • \$\begingroup\$ Not sure what's the problem pastebin.com/eX7uNreC \$\endgroup\$
    – bushed
    Jan 24, 2017 at 11:07
  • 2
    \$\begingroup\$ 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. \$\endgroup\$
    – bushed
    Jan 24, 2017 at 11:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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