macOS Is there a way to clean up this Java method?

[ArtOfWarfare]

This code looks highly repetitive and I suspect there's a much cleaner way of writing it, but it's not coming to mind. Does anyone have any suggestions?

(It's title() that concerns me, not dividesWell() or format(), although I'm open to any suggestions that anyone has for either of those. I just included them for completeness, since title() references both of them.)

/** 
 * Determines a title based on simplifying the units of the duration.
 */
private String title(long seconds) {
    long minutes = dividesWell(seconds, 60);
    if (minutes != 0) {
        long hours = dividesWell(minutes, 60);
        if (hours != 0) {
            long days = dividesWell(hours, 24);
            if (days != 0) {
                long years = dividesWell(days, 365);
                if (years != 0) {
                    return format(years, "Year");
                }
                long weeks = dividesWell(days, 7);
                if (weeks != 0) {
                    return format(weeks, "Week");
                }
                return format(days, "Day");
            }
            return format(hours, "Hour");
        }
        return format(minutes, "Minute");
    }
    return format(seconds, "Second");
}

/**
 * Returns round(a/b) if it's a close approximate of a/b, or 0 if it's not.
 */
private long dividesWell(long a, long b) {
    double actual = (double)a/(double)b;
    long approximate = Math.round(actual);
    double error = Math.abs((actual - approximate) / actual);
    return error < 0.009 ? approximate : 0;
}

/**
 * Writes the number and the unit, pluralized if appropriate.
 */
private String format(long a, String unit) {
    return a + " " + unit + (a != 1 ? "s" : "");
}

 

[zeppenwolf]

I'm not sure I got the logic zactly right, and I don't know java... But I like to push code to the left whenever possible, so if I can return from a f() by flipping the logic I'll do it. Probably not what you meant, (!) but to me nested blocks are the hardest thing to read:

private String title(long seconds) {

    long minutes, hours, days, years, weeks;

    if ( ! (minutes = dividesWell(seconds, 60)) )
        return format(seconds, "Second");

    if ( ! (hours = dividesWell(minutes, 60)) )
        return format(minutes, "Minute");

    if ( ! (days = dividesWell(hours, 24)) )
        return format(hours, "Hour");

    if ( years = dividesWell(days, 365) )
        return format(years, "Year");
    else {
        if ( weeks = dividesWell(days, 7) ) )
            return format(weeks, "Week");
        else
            return format(days, "Day");
    }

    return format(seconds, "Second");
}

 

[ArtOfWarfare]

Java disallows using numbers directly in conditional statements (you have to do != 0 to achieve the same thing) but I get the gist of what you did.

I'll have to try it out. I concur that nested blocks are terrible and are exactly what I was looking to remove.

At the same time, using assignment in conditionals is also generally frowned upon. I have to decide which is a lesser of two evils. Unless someone has something cleaner still.

Maybe some kind of function that somehow combines the dividesWell(), conditional, and format() all in one and lets me somehow chain them together?

 

[lee1210]

private static final double CLOSENESS_FACTOR = 0.009;
private static final Map<Long,String> NUM_SECONDS_TO_UNIT = makeSecondToUnitMap();

private static Map<Long, String> makeSecondToUnitMap() {
  final Map<Long, String> result = new LinkedHashMap<Long,String>();
  result.put(7L*TimeUnit.DAYS.toSeconds(1),"Week");
  result.put(TimeUnit.DAYS.toSeconds(1),"Day");
  result.put(TimeUnit.HOURS.toSeconds(1),"Hour");
  result.put(TimeUnit.MINUTES.toSeconds(1),"Minute");
  return Collections.unmodifiableMap(result);
}

private static final Map<Long, String> TIME_DENOMINATION_TO_UNIT = makeTimeDenominationToUnitMap();

private static Map<Long, String> makeTimeDenominationToUnitMap() {
  final Map<Long, String> result = new LinkedHashMap<Long,String>();
  result.put(TimeUnit.MINUTES.toSeconds(1),"Minute");
  result.put(TimeUnit.HOURS.toMinutes(1),"Hour");
  result.put(TimeUnit.DAYS.toHours(1),"Day");
  result.put(7L,"Week");
  return Collections.unmodifiableMap(result);
}


/** 
 * Determines a title based on simplifying the units of the duration.
 */
private String title(long seconds) {
  for(Map.Entry<Long,String> secondsToUnit : NUM_SECONDS_TO_UNIT.entrySet()) {
    final long numSecondsForUnit = secondsToUnits.key();
    final long approximateResult = approximatelyDivide(seconds,numSecondsForUnit);
    if(approximateResult != 0) {
      return format(approximateResult, secondsToUnit.value());
    }
  }
  return format(seconds,"Second");
}

private String titleTwo(long seconds) {
  String lastUnit = "Second";
  long lastValue = seconds;
  for(Map.Entry<Long,String> divisorToUnit : TIME_DENOMINATION_TO_UNIT.entrySet()) {
    final long newDivisor = divisorToUnit.key();
    final long newValue = approximatelyDivide(lastValue,newDivisor);
    if(newValue == 0L) {
      return format(lastValue, lastUnit);
    }
    lastUnit = divisorToUnit.value();
    lastValue = newValue;
  }
  return format(lastValue,lastUnit);
}
  

/**
 * Returns round(a/b) if it's a close approximate of a/b, or 0 if it's not.
 */
private long approximatelyDivide(long numerator, long denominator) {
  if(denominator == 0L) {
    throw new InvalidArgumentException("Cannot divide by 0");
  }
  
  final double exactResult = (double) numerator/(double) denominator;
  final long roundedResult = Math.round(exactResult);
  double errorMagnitude = Math.abs(1.0 - (roundedResult / exactResult));
  return (errorMagnitude < CLOSENESS_FACTOR) ? 
    roundedResult : 
    0;
}

/**
 * Writes the number and the unit, pluralized if appropriate.
 */
private String format(long value, String unit) {
  final String pluralize = (value != 1L) ? "s" : "";
  return String.format("%d %s%s", value, unit, pluralize);
}

I didn't precondition all of this, but some > 0 checks and null checks would be helpful.

I really wasn't sure what the hopes were with the magnitude of error check. I left it intact (I wrote a nearly all long version to avoid the double ugliness, but it was long). titleTwo does something closer to your original. title goes the opposite direction, trying to fit the largest unit first. I'm guessing you're wanting the error to accumulate as you climb, though.

Probably didn't name things well enough. Hopefully the gist is clear. Generate a static map at class load to track the numbers to the name of the unit, then walk the map calculating. The nesting only goes two deep and there's not a lot of layers of temporary state.

May not stand up to muster under code review at work, but for the middle of the night on my phone, it will do.

-Lee

 

[robvas]

Looks like 'titleize' functions I've seen in other languages. Leave it alone.

 

[gnasher729]

I just find the results strange. You'll display for some consecutive numbers "50 minutes", "50 minutes", "50 minutes", "3028 seconds", "3029 seconds", "3030 seconds", "3031 seconds", "3032 seconds", "51 minutes", "51 minutes".

I think I would do something simpler, like up to 150 seconds, then 3 to 150 minutes. 3 to 60 hours, 3 to 17 days, 3 to 10 weeks, then months. So as the time gets longer, units would always go up, not forth and back.

 

[ArtOfWarfare]

I just find the results strange. You'll display for some consecutive numbers "50 minutes", "50 minutes", "50 minutes", "3028 seconds", "3029 seconds", "3030 seconds", "3031 seconds", "3032 seconds", "51 minutes", "51 minutes".

I think I would do something simpler, like up to 150 seconds, then 3 to 150 minutes. 3 to 60 hours, 3 to 17 days, 3 to 10 weeks, then months. So as the time gets longer, units would always go up, not forth and back.

I do want to go back and forth, though. I want to display 2 weeks instead of 14 days, but 90 days instead of 13 weeks, as an example.