5
\$\begingroup\$

I have date range array and a removeRange object which can split or remove portion of on the date range array. If Range is within the date range it will split the record into 2 range otherwise it can trim portion of the date range or remove entirely the date range. I've moment js on the date query to check of date is greater than or less than. SnippetLink

var appt = [{
  text: 'appointment 1',
  StartTime: new Date('June 27,2017 08:00:00'),
  EndTime: new Date('June 27,2017 10:00:00'),
  deleted: false
}, {
  text: 'appointment 2',
  StartTime: new Date('June 28,2017 08:00:00'),
  EndTime: new Date('June 28,2017 12:00:00'),
  deleted: false
}, {
  text: 'appointment 3',
  StartTime: new Date('June 28,2017 13:00:00'),
  EndTime: new Date('June 28,2017 17:00:00'),
  deleted: false
}];
console.log(appt);

//NOTE: this will remove appointment 2 and 3
//var removeRange = {
// StartTime: new Date('June 28,2017 05:00:00'),
//EndTime: new Date('June 28,2017 18:00:00')
//}

//NOTE: This will split the appointment 2 
//var removeRange = {
//  StartTime: new Date('June 28,2017 09:00:00'),
//  EndTime: new Date('June 28,2017 10:00:00')
//}

//NOTE: This will remove portion of appointment 2 and appointment 3
var removeRange = {
  StartTime: new Date('June 28,2017 09:00:00'),
  EndTime: new Date('June 28,2017 14:00:00')
}

var forAddLater = [];
appt.forEach(function(appointment, index) {
  if (moment(removeRange.StartTime).isSame(moment(appointment.StartTime), 'day')) {
    //console.log('same day', moment(appointment.StartTime));

    if (moment(removeRange.StartTime).isSameOrBefore(moment(appointment.StartTime))) {
      console.log('same or before', moment(removeRange.StartTime));

      if (moment(removeRange.EndTime).isBefore(moment(appointment.EndTime)) &&
        moment(removeRange.EndTime).isAfter(moment(appointment.StartTime))) {
        console.log('removeRange.EndTime is before appt end time');
        console.log('New StartTime is removeRange.EndTime');
        appt[index].StartTime = removeRange.EndTime;
      } 

      if(moment(removeRange.EndTime).isSameOrAfter(moment(appointment.EndTime))){
        console.log('For delete the appt');
        appt[index].deleted = true;
      }
      //else {
        //if (moment(removeRange.EndTime).isBefore(moment(appointment.EndTime))) {
        //  appt[index].StartTime = removeRange.EndTime;
        //} else {
        //  console.log('For delete the appt');
        //  appt[index].deleted = true;
        //}

      //}
    } else {
      console.log('inside the range');
            console.log(moment(removeRange.EndTime));
      console.log(moment(appointment.EndTime));
      if (moment(removeRange.EndTime).isBefore(moment(appointment.EndTime))) {
        console.log('split the appointment data and add new');
        forAddLater.push({
          text: appointment.text + '-split',
          StartTime: removeRange.EndTime,
          EndTime: appointment.EndTime,
          deleted: false
        });
        appt[index].EndTime = removeRange.StartTime;
      } else {
        console.log(appointment);
        console.log('remove the end portion only');
        appt[index].EndTime = removeRange.StartTime;
      }
    }

  }
});

appt.push(forAddLater);

appt = appt.filter(function(el) {
  return !el.deleted;
});

console.log(appt);

Is there a better enhancement on this or a little bit simpler way to handle this one? Is there a possible scenario where the code may fail?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

As it is, this code will need quite a bit of work. If I understood the requirements correctly, what you want to do is basically removing all the overlap of the removeRange from the given appointments.

We can think about this using timelines (which I find a bit easier than words for this case). In summary there is basically five different cases to look over:

  1. There is no overlap between the removeRange and the Appointment:

    Appt.:      s---e
    Rem1.:  s---e
    Rem2.:          s--e
    

    Note that I am explicitly including the case that the removed enddate and the startdate of the appointment (or vice versa) are equal. This fuzzy "inside" behaviour allows us to do less work more often (and sidesteps the issue of defining the smallest timeunit).
    The code presented in the question does seem to be doing the same.

  2. The removeRange completely covers the Appointment:

    Appt.:    s---e
    Rem. :  s-------e
    

    This case is also somewhat easy to handle: The appointment is dropped completely.

  3. The removeRange overlaps the start, but not the end. The appointment's StartTime is modified to match the removeRange's EndTime.

  4. Or the opposite case of overlapping end, but not start.

  5. The removeRange is somewhere in the middle of the appointment, splitting it in two:

    Appt.:  s--------e
    Rem. :     s--e
    

A helpful way of thinking about this is using moment's isBetween. We can create a simple truth table and label the cases.

A: appointment start between removeRange bookends
B: appointment end between removeRange bookends $$ \begin{array}{l r r} \text{Description} & A & B \\ \text{No overlap (1) OR Splitting} & f & f \\ \text{End overlaps (4)} & f & t \\ \text{Start overlaps (3)} & t & f \\ \text{Remove appointment (2)} & t & t \end{array} $$

This setup allows a simplification of the control structure setup you have into something that doesn't need that much information:

// pseudocode, because there's some more improvements
const removeCoversStart = appointment.StartTime.isBetween(removeRange.StartTime, removeRange.EndTime);
const removeCoversEnd = appointment.EndTime.isBetween(removeRange.StartTime, removeRange.EndTime);

if (removeCoversStart && removeCoversEnd) {
   // delete appointment
} else if (removeCoversStart && !removeCoversEnd) {
  // update start to removal end
} else if (!removeCoversStart && removeCoversEnd) {
  // update end to removal start
} else {
  // no overlap or split
}

Note that the order of comparisons here is a bit non-optimal and there is some duplication in how the booleans we use for determining our next step are calculated. This can be somewhat remedied with the following helper:

function isAffected(appointmentDate, removeRange) {
    return moment(appointmentDate).isBetween(removeRange.StartTime, removeRange.EndTime);
}

Together with a little reordering that turns the code from the question into:

const forAddLater = [];
appt.forEach(function(appointment) {
  const removeCoversStart = isAffected(appointment.StartTime, removeRange);
  const removeCoversEnd = isAffected(appointment.EndTime, removeRange);

  if (!(removeCoversStart || removeCoversEnd)) {
    if (isAffected(removeRange.StartTime, appointment)) {
      // removeRange is splitting the appointment in two
    }
    return;
  }
  if (removeCoversStart && removeCoversEnd) {
    // using index access into the original array is not necessary
    appointment.deleted = true;
    return
  }
  // at this point we only need to adjust one of the times
  if (removeCoversStart) { // implying !removeCoversEnd
    appointment.StartTime = removeRange.EndTime;
  } else { // removeCoversEnd && !removeCoversStart
    appointment.EndTime = removeRange.StartTime;
  }
});

Closing thoughts:

The code presented in the question is a bit difficult to logically follow. That's partly due to the lack of a thought-out approach to the problem. Another issue is the copious logging output (which is just noise when trying to understand the algorithm) and the repeated conversion of dates to moments.

It does not help that the indentation is kind of small and the structural blocks of the code are made larger than necessary.

This code can benefit a lot from extracting variables and putting repeated logic (as well as possibly the interactions with moment) into separate helper functions.

Another thing that might be worth considering is making the appointments immutable and generating an output array instead of modifying the existing appt array. That also has the advantage of not modifying "borrowed" data.

\$\endgroup\$

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.