//retrieves the datetime WITHOUT loading the whole image
string[] GetDateTakenFromImage(string path)
Function name and parameter are named well. You should use an XML-doc header to document the quirk that it doesn't need to load the entire image.
Overall I like that the method is concise: it mostly does only one thing, it's easy to read, and not needing to pre-load the whole image is a nice bonus.
It is strange to use string[]
to denote a date. You should be returning a DateTime?
.
Consider changing it to accept a Stream
instead of a string path
. Currently it's a bit burdensome to test your method because it requires a file path, even though all it's doing is getting a stream out of it anyway. By accepting a Stream
instead, you can more easily put automated tests around it that use in-memory test data and avoid a whole litany of IO nonsense.
using (FileStream fs = new FileStream(path, FileMode.Open, FileAccess.Read))
fs
is a poor name. Give it some more meaning, like imageStream
. It can also be written a bit more concisely:
using (FileStream imageStream = File.OpenRead(path))
Likewise, myImage
could just be named image
.
PropertyItem propItem = myImage.GetPropertyItem(36867);
Avoid magic numbers -- that 36867 should be in a named constant somewhere:
const int ExifDateTimeOriginal = 0x9003;
Your error handling in general could be improved. If I was consuming this API, I would naturally expect exceptions relating to IO (file not found, file not accessible, not a valid image file, and so on) to propagate up. It's up to you whether you want to throw or return null
in the case where everything is valid but the image simply doesn't have that tag.
You're returning null
if anything goes wrong which makes this harder to test. Be aware that myImage.GetPropertyItem(36867);
will throw if the tag is not present (which in my opinion is a totally non-exceptional circumstance), so if you do tweak your method to propagate other exceptions you will need to put that one line around a try-catch for that one exception.
The EXIF tag you're checking is encoded in ASCII according to the EXIF docs I've been able to find, so this should use Encoding.ASCII
instead of Encoding.UTF8
:
string dateTaken = r.Replace(Encoding.UTF8.GetString(propItem.Value), "-", 2);
You also don't need to do any string replacing. DateTime.ParseExact
is handy for parsing dates encoded in custom formats:
string dateTaken = Encoding.ASCII.GetString(propItem.Value);
...
return DateTime.ParseExact(dateTaken.Trim('\0'), "yyyy:MM:dd HH:mm:ss", CultureInfo.InvariantCulture);
Lastly, if you want to really adhere to the letter of the spec then depending on how you decide to modify or keep your method contract you'll need to handle the case where the date and time are unknown and all non-colon characters are replaced with spaces.