Is there a way to clean up this Java method?

Discussion in 'Mac Programming' started by ArtOfWarfare, Nov 23, 2013.

  1. ArtOfWarfare macrumors 604

    ArtOfWarfare

    Joined:
    Nov 26, 2007
    #1
    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.)

    Code:
    /** 
     * 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" : "");
    }
     
  2. zeppenwolf macrumors regular

    zeppenwolf

    Joined:
    Nov 17, 2009
    #2
    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:

    Code:
    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");
    }
     
  3. ArtOfWarfare thread starter macrumors 604

    ArtOfWarfare

    Joined:
    Nov 26, 2007
    #3
    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?
     
  4. lee1210 macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #4
    Code:
    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
     
  5. robvas macrumors 68020

    Joined:
    Mar 29, 2009
    Location:
    USA
    #5
    Looks like 'titleize' functions I've seen in other languages. Leave it alone.
     
  6. gnasher729, Nov 25, 2013
    Last edited: Nov 25, 2013

    gnasher729 macrumors P6

    gnasher729

    Joined:
    Nov 25, 2005
    #6
    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.
     
  7. ArtOfWarfare thread starter macrumors 604

    ArtOfWarfare

    Joined:
    Nov 26, 2007
    #7
    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.
     

Share This Page