I have the following code snippet that I would like to optimize:

import org.joda.time.{DateTime, Days, Months, Weeks, Years}

  @scala.annotation.tailrec
  def optimize(start: DateTime, end: DateTime, acc: List[(DateTime, DateTime)]): List[(DateTime, DateTime)] = {

    def doDaysExist(startDate: DateTime, endDate: DateTime): Boolean = {
      Days.daysBetween(startDate, endDate).getDays != 0
    }

    if (Years.yearsBetween(start, end.plusDays(1)).getYears != 0) {
      val years = Years.yearsBetween(start, end.plusDays(1)).getYears
      val newStart = start.plusYears(years).minusDays(1)
      if (doDaysExist(newStart, end)) {
        optimize(newStart, end, (start, newStart)  : acc)
      } else (start, end)  : acc
    } else if (Months.monthsBetween(start, end.plusDays(1)).getMonths != 0) {
      val months = Months.monthsBetween(start, end.plusDays(1)).getMonths
      val newStart = start.plusMonths(months).minusDays(1)
      if (doDaysExist(newStart, end)) {
        optimize(newStart, end, (start, newStart)  : acc)
      } else (start, end)  : acc
    } else if (Weeks.weeksBetween(start, end.plusDays(1)).getWeeks != 0) {
      val weeks = Weeks.weeksBetween(start, end.plusDays(1)).getWeeks
      val newStart = start.plusWeeks(weeks).minusDays(1)
      if (doDaysExist(newStart, end)) {
        optimize(newStart, end, (start, newStart)  : acc)
      } else (start, end)  : acc
    } else {
      // For sure days
      (start, end)  : acc
    }
  }

As it can be seen that the if condition and the next statement that follows are kind of duplicate code. Is there a better way to make it better and cryptic? I have to step through this hierarchy as I have to first check the year, then the month, then the week and then the day.

I can think of extracting the repeated code block into lambdas, but is that good enough? I can't think of rewriting it as a Map as that code block runs in a tail recursive function.

CodePudding user response：

What about something like this:

def optimize(start: DateTime, end: DateTime, acc: List[(DateTime, DateTime)]): List[(DateTime, DateTime)] = {
  val endPlusDay = end.plusDays(1)
  
  val checks = List(
    {
      val years = Years.yearsBetween(start, endPlusDay).getYears
      val newStart = start.plusYears(years).minusDays(1)
 
      (years -> newStart)
    },
    {
      val months = Months.monthsBetween(start, endPlusDay).getMonths
      val newStart = start.plusMonths(months).minusDays(1)
 
      (months -> newStart)
    },
    {
      val weeks = Weeks.weeksBetween(start, endPlusDay).getWeeks
      val newStart = start.plusWeeks(weeks).minusDays(1)
 
      (weeks -> newStart)
    }
  )
  
  checks.collectFirst {
    case (difference, newStart) if (
      (difference != 0) && (Days.daysBetween(newStart, end) != 0)
    ) =>
      optimize(newStart, end, acc :  (start, newStart))
  }.getOrElse(acc :  (start, end))
}

PS: Two recommendations

1. Use java.time instead of Joda.
2. Prepend and finally reverse to the accumulator instead of append.

CodePudding user response：

Just pre-compute the values and move the second test outside the if/else:

val years = Years.yearsBetween(start, end.plusDays(1)).getYears
lazy val months = Months.monthsBetween(start, end.plusDays(1)).getMonths
lazy val weeks = Weeks.weeksBetween(start, end.plusDays(1)).getWeeks

val newStart =
  if (years != 0) {
    start.plusYears(years).minusDays(1)
  } else if (months != 0) {
    start.plusMonths(months).minusDays(1)
  } else if (weeks != 0) {
    start.plusWeeks(weeks).minusDays(1)
  } else {
    start
  }

if (doDaysExist(newStart, end)) {
  optimize(newStart, end, (start, newStart)  : acc)
} else {
  (start, end)  : acc
}

Using lazy may or may not improve performance so measure to see which is best.