JakeWharton / DiskLruCache

Java implementation of a Disk-based LRU cache which specifically targets Android compatibility.
http://jakewharton.github.io/DiskLruCache
Apache License 2.0
5.79k stars 1.18k forks source link

Feature Request: make DiskLruCache.Editor Closeable #5

Closed orip closed 12 years ago

orip commented 12 years ago

In order to always match an edit to a commit() or abort(), even in case of exceptions, it would be nice to be able to do this:

Editor e = cache.edit(key);
try {
  // edit values
  e.commit();
} finally {
  e.close(); // will abort(), unless the editor has already been committed or aborted
}

I can submit a pull request if preferred.

swankjesse commented 12 years ago

My main gripe is that calling close() isn't necessary. Another way to write the code above is:

boolean success = false;
Editor e = cache.edit(key);
try {
  // edit values
  e.commit();
  success = true;
} finally {
  if (!success) e.abort();
}

I can't claim that this is any better. But I do think it's more intention-revealing.

orip commented 12 years ago

@swankjesse What bothers me with the current interface is that I have to maintain the success state in every scenario that I edit keys. If it's inherent to editing, I lean towards the Editor having explicit support for this state.

An alternative would be just keeping and exposing the state instead of creating the .close() logic:

...
} finally {
  if (!e.wasCommitted())
    e.abort();
}

but making the Editor closeable feels elegant to me, and I think maps to Java's idioms better.

swankjesse commented 12 years ago

Do we all hate this name?

e.abortUnlessCommitted() ?

I'm coming around to close(), but I still dislike the fact that it makes a naked commit() look leaky.

JakeWharton commented 12 years ago

I like it. Close doesn't exactly convey what's happening behind the scenes.

orip commented 12 years ago

That's a really good name.

JakeWharton commented 12 years ago

Merged to dev.