Move file name based I/O to extension methods for facilitated adaptation to PCL

[anders9ustafsson]

NOTE! The proposal in this comment is obsolete! See this comment for updated proposal.

The .NET file I/O operations in the System.IO namespace are generally not applicable to other platforms.

PCLStorage provides a platform agnostic abstraction layer for common file I/O operations.

To facilitate porting fo-dicom to other platforms such as Windows 8/Windows Phone 8.1/Universal Windows 10 as well as Xamarin iOS and Xamarin Android, I suggest that the internal calls to System.IO.File, System.IO.Directory etc. should be refactored to use PCLStorage constructs instead.

Please share your thoughts on this issue.

[IanYates]

Sounds nice. Given the async/await that's all through it I suppose this depends on #65 being done first. Have you used this library for your fo-DICOM portable fork? Any drawbacks or other weird things?

[anders9ustafsson]

NOTE! The proposal in this comment is obsolete! See this comment for updated proposal.

@IanYates It is not necessary that #65 is done first, it is possible to (temporarily) apply synchronous embeddings on the PCLStorage calls. Personally I would prefer to implement this task first, I think it is easier to overview the implications of async/await with the PCLStorage implementation already in place.

In my portable fork, I have chosen another strategy, namely to keep the original fo-dicom source code as intact as possible, and instead use e.g. the File and Directory implementations from my Shim project. But Shim was designed to provide implementations of unported .NET framework APIs for the particular purpose of not having to change .NET based legacy code. fo-dicom on the other hand is a "moving target" which we can adapt and modernize to our needs. We are free to replace un-ported method calls if we see fit. I would like to move in this direction, but of course I want it to be an unanimous decision between us project administrators.

So far I have not used PCLStorage extensively, but it is a well established project by renowned MS employee and PCL expert Daniel Plaisted, so I am fairly confident that it should well fit our needs.

[IanYates]

@anders9ustafsson - sounds good.
With the temporary synchronous embeddings, what do you mean by that? Do you mean blocking on access to the returned Task's Result property?
I'd too saw it was by Daniel Plaisted and automatically gave the library a thumbs up in my mind from a quality point of view :) Since it will be a compulsory third party dependency of fo-dicom the choice of library is important. It's also good that it's not something that's constantly evolving so we shouldn't have to worry too much about needing to IL Merge it or anything, whereas many NuGet packages merge in things like JSON.Net to avoid breaking changes between its major versions.

[anders9ustafsson]

NOTE! The proposal in this comment is obsolete! See this comment for updated proposal.

I have used the following construct for example in Shim:

var returnValue = Task.Run(async () => await GetReturnValueAsync(x, y)).Result;

which means that you limit the async/await usage to inside the Task.Run statement. I cannot say for sure that this construct is generally applicable, but I think it should work sufficiently in most contexts where external synchronicity is required.

[anders9ustafsson]

@IanYates @GMZ @hdesouky I have given this issue some more thought, and I would like to modify the proposal. Rather than adapting the internal file based I/O operations to use portable constructs, I suggest that we make the core I/O operations entirely Stream based instead, and turn the relevant file based I/O operations into extension methods.

Example
Replace the DicomFile.Save(string) method with an extension method DicomFileExtensions.Save(this DicomFile, string), with an implementation similar to this:

public static class DicomFileExtensions {
  public static void Save(this DicomFile dicomFile, string fileName) {
    using (var stream = File.OpenWrite(fileName)) {
      dicomFile.Save(stream);
    }
  }
}

In the case of DicomFile.Save, there already exists a Stream argument overload; in other cases we would replace an instance method SomeMethod(string fileName) with SomeMethod(Stream stream) and implement a static class SomeMethodExtensions containing the corresponding file name based convenience method.

Using this approach it would be much easier to eventually split the DICOM assembly into a Stream based core assembly and a separate file I/O assembly. Stream is completely portable to all target platforms, so porting fo-dicom I/O to e.g. Universal Windows or Xamarin iOS/Android would then be limited to adapting the file I/O assembly to the specific platform.

Please also note that these modifications can be made without adding any third-party library dependencies to fo-dicom.

What do you think? Would it be worthwhile if I prepared a pull request to demonstrate this proposal in practice?

[IanYates]

@anders9ustafsson - that does seem like a sensible approach and it's how I handle some of my API code internally when I need to read from disk or from a stream.

I haven't thought it through completely yet but are there places in fo-dicom that only take a file or byte[] at the moment? (I don't have the code handy)
If they're changed to use Stream, which is a good change, we'll need to make sure there's no need for random access or, if there is, check up front when getting the stream that CanSeek is true. And if we are seeking around, we'll need to be careful to check the Stream position.

Similarly, are there places in fo-dicom that would expect the stream to now be "owned" and thus automatically disposed?

[anders9ustafsson]

Many thanks, @IanYates , these items will definitely need to be kept in mind when refactoring the code. I cannot say for sure right now if there are any random access parts that are not already using Stream, but I will move carefully when changing the code. And the same goes for ownership, of course.