Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Premise

Consider the following method:

static String formatMyDate(Date date) {
    return new SimpleDateFormat("yyyy-MM-dd").format(date);
}

It's often desirable to memoize DateFormat objects so they can be reused rather than repeatedly instantiating new ones. This frequently leads to the following naive refactor:

private static final DateFormat DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd");

static String formatMyDate(Date date) {
    return DATE_FORMAT.format(date);
}

But this is wrong for multithreaded applications. From the DateFormat documentation:

Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.

Assuming our application is using thread pooling, this leads us to memoize an object per thread, using ThreadLocal (see article):

private static final ThreadLocal<DateFormat> DATE_FORMAT_REF =
        new ThreadLocal<DateFormat>() {
            @Override
            protected DateFormat initialValue() {
                return new SimpleDateFormat("yyyy-MM-dd");
            }
        };

static String formatMyDate(Date date) {
    return DATE_FORMAT_REF.get().format(date);
}

This works fine. But I've noticed it can still leave code duplication across a larger project. For example multiple developers could each write their own code using "yyyy-MM-dd" date formats in various classes. This led me to write the following helper class (using JSR-305 and Guava):

Solution 1

public class DateFormats {

    private static final ThreadLocal<Map<String, DateFormat>> DATE_FORMAT_MAP_REF =
            new ThreadLocal<Map<String, DateFormat>>() {
                @Override
                protected Map<String, DateFormat> initialValue() {
                    return Maps.newHashMap();
                }
            };

    private DateFormats() { }

    /**
     * Retrieves, and if necessary creates and caches, a {@code DateFormat} instance
     * corresponding to the specified format string.
     * 
     * @param dateFormatString The date format string.
     * @return The date format.
     */
    public static DateFormat get(String dateFormatString) {

        Preconditions.checkNotNull(dateFormatString, "dateFormatString");

        final Map<String, DateFormat> dateFormatMap = DATE_FORMAT_MAP_REF.get();
        @Nullable DateFormat dateFormat = dateFormatMap.get(dateFormatString);
        if (dateFormat == null) {
            dateFormat = new SimpleDateFormat(dateFormatString);
            dateFormatMap.put(dateFormatString, dateFormat);
        }
        return dateFormat;
    }
}

And the usage would look like:

static String formatMyDate(Date date) {
    return DateFormats.get("yyyy-MM-dd").format(date);
}

One downside to this solution is that a new HashMap gets created for every calling thread. I'm considering using a Guava LoadingCache to implement the following alternative:

Solution 2

public class DateFormats {

    private static final LoadingCache<String, ThreadLocal<DateFormat>> DATE_FORMAT_REF_CACHE =
            CacheBuilder.newBuilder()
            .concurrencyLevel(4) //how to compute this is up for debate
            .build(new CacheLoader<String, ThreadLocal<DateFormat>>() {
                @Override
                public ThreadLocal<DateFormat> load(final String dateFormatString) {
                    return new ThreadLocal<DateFormat>() {
                        @Override
                        protected DateFormat initialValue() {
                            return new SimpleDateFormat(dateFormatString);
                        }
                    };
                }
            });

    private DateFormats() { }

    /**
     * ...
     */
    public static DateFormat get(String dateFormatString) {

        Preconditions.checkNotNull(dateFormatString, "dateFormatString");

        return DATE_FORMAT_REF_CACHE.getUnchecked(dateFormatString).get();
    }
}

Questions

  • Is the premise of this solution too pedantic, or is there value in implementing either solution?
  • Which solution is preferable for a web-scale application?
  • Is there anything about the solutions you would fix or otherwise change?
  • For solution 2, what are ways to better calculate concurrency level?

I haven't had a chance to write unit tests, but will post them if requested when time allows.

share|improve this question
2  
Is there a reason you have to use java.util.Date instead of JodaTime? The JodaTime library has a DateTimeFormat class that is thread-safe. As a side note, you can look at the source for how they implemented it as well. –  Atif Feb 18 '13 at 19:04
    
I've recommended adopting JodaTime to my team members on multiple occasions but it's simply not considered a priority. Thanks for recommending looking at their source though, I didn't think to do that. –  Paul Bellora Feb 18 '13 at 19:15
1  
I would like to thank you for you very well-written question, it was both informative and interesting. My advice: use the more elegant LoadingCache solution (maybe with another wrapper to make the usage simpler), and don't worry about tuning the parameters until you've shown it slows the application down. –  Quentin Pradet Feb 20 '13 at 7:09

2 Answers 2

up vote 3 down vote accepted

It looks like the questions are very specific. You should probably formulate a goal. Is your main goal to reduce code duplicity? Memory usage? Speed (however we define it here)? Something else? A combination of previous items? (which makes life harder the more items you combine) This affects the first two questions.


For the first question, I could imagine, that you have some common date formats. If less than 5 formats make up to 90% of usage, I would supply a specific method like

/** 
 * Formats the date exactly the same as SimpleDateFormat("yyyy-MM-dd").format(date),
 * but is multi threaded safe without any penalties in subsequent calls.<br>
 * Example: 2012-09-30
 */
public static String (DateFormats.)formatDate_yyyy_MM_dd(Date date) { ... }

You could use ThreadLocal then.
This would solve this the easiest way. If you need any more sophisticated solution, you could change the class internals without breaking any external code.


For the last 2 questions, I do not have experience with LoadingCache, so I cannot help there.
I would have used a standard HashMap to store the ThreadLocal Version of DateFormat. As far as I thought about it, we do not need synchronization for the map, because we do not want to replace entries, we do not care if we put something multiple times in rare cases for the initialization part of a new datestring and we do not remove anything.


A bit offtopic, but: One thing I do not get about your description. If they do not care about using yoda (which could be used inside this DateFormats class and no one would notice about it), how could you convince them to use this DateFormats class?

share|improve this answer
    
Thanks for the answer. I have a few questions, but I'm no Java expert. 1/ I don't understand how hardcoding the format in the method name makes things better. 2/ Why avoid LoadingCache only because you don't kow it? 3/ A 99% correct class is still not correct. You don't know how SimpleDateFormat is implemented, maybe format() itself isn't thread-safe? 4/ I think they will be more likely to use something designed by they coworker than something they don't know from a library. Close to the NIH syndrome. –  Quentin Pradet Feb 20 '13 at 7:15
    
+1 for the intuitive formatting Date method, thread safe. –  cl-r Feb 20 '13 at 7:29
    
@Cygal: 1) It abstracts from the implementation. A caller knows what is happening, implementation could be done inside the method. Same as for example toBinaryString() from Integer. 2) I did not say to avoid it, I just can not answer the two questions from my past experience. 3) format() is not thread safe. The idea is to have a not-synchronized map, instead of the LoadingCache. The DateFormat must be safe. 4) I do not know, I just wanted to point at this thought, because it could affect the amount of work you spent into this. -- I have edited my post to clarify some roots for your questions –  tb- Feb 20 '13 at 9:59
    
Thanks for your answer. I did consider the idea with the individual methods, but the naming would've been awkward for some formats and I didn't want it to be javadoc-dependent. Interesting point about just using HashMap instead of a Cache. –  Paul Bellora Feb 22 '13 at 12:09
    
Yes, it could be an option only if you have a few easy main cases. I strongly agree to not start a 200 method class for different dates. –  tb- Feb 22 '13 at 13:36

Since I'm new here I can't comment but http://stackoverflow.com/questions/5038169/synchronizing-on-simpledateformat-vs-clone brings up the excellent idea of pooling the formatters instead. Have you tried that and seen differences between using a ThreadPool and a regular pool? If there's no difference perhaps it's better to use a general pool as you can then switch approach to how short/long lived threads you use without reimplementing your pool.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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