Return to Answer

• 1

• 29k
• 11
• 86
• 188

• 50.9k
• 5
• 83
• 177

• You named your class MediaFile but basically its a class which creates checksums for any given file. You should consider to rename the class to e.g ChecksumCreator of FileChecksumCreator. This would make the purpose and the responsibility of that class obvious by looking at its name.

• Omitting braces {} although they might be optional can lead to serious bugs which aren't tracked easily. I would like to encourage you to always use them.

• You are using C# 6 but you still use private setters for your properties. With C# 6 you can just omit them if you either set the proerties at startup or from inside of the constructor. The backing filed of a getter only property is internal handled as readonly and because you only set them inside the constructor you should take this route. While we are at the constructor, you are first assigning null to the Checksums property and later on you are using the GenerateChecksums() method to assign a value to it. The mentioned points could be implemented like so

   public string FilePath { get; }
   public List<byte[]> Checksums { get; }
   private FileInfo fileInfo;
   private MemoryMappedFile memoryMappedFile;
   public MediaFile(string filePath)
   {
   if (!File.Exists(filePath))
   {
   throw new FileNotFoundException($"{filePath} was not found!");
   }
   this.FilePath = filePath;
   this.fileInfo = new FileInfo(filePath);
   this.memoryMappedFile = MemoryMappedFile.CreateFromFile(filePath, FileMode.Open);
   this.Checksums = this.GenerateChecksums();
   } 
  

but basically you won't need a FileInfo object here, because you are only using the Length property of the FileInfo object. So let us change this like so

 public string FilePath { get; }
 public List<byte[]> Checksums { get; }
 private long fileLength;
 private MemoryMappedFile memoryMappedFile;
 public MediaFile(string filePath)
 {
 if (!File.Exists(filePath))
 {
 throw new FileNotFoundException($"{filePath} was not found!");
 }
 this.FilePath = filePath;
 this.fileLength= new FileInfo(filePath).Length;
 this.memoryMappedFile = MemoryMappedFile.CreateFromFile(filePath, FileMode.Open);
 this.Checksums = this.GenerateChecksums();
 } 

But what happens if you decide later on to go the way @EricLippert has shown in the comments, like using a CRC hashprovider to generate the checksums ? You would need to change the class in question. Maybe it would be better to inject a HashAlgorithm provider so you could easily switch to another one.

This would result in an overloaded constructor and a slightly changed Dispose() method like so

 private bool hasOwnershipOfHashprovider = false;
 private HashAlgorithm hashProvider;
 public MediaFile(string filePath)
 : this(filePath, MD5.Create())
 {
 hasOwnershipOfHashprovider = true;
 }
 public MediaFile(string filePath, HashAlgorithm hashProvider)
 {
 if (!File.Exists(filePath))
 {
 throw new FileNotFoundException($"{filePath} was not found!");
 }
 if (hashProvider == null)
 {
 throw new ArgumentNullException($"hashProvider");
 }
 this.FilePath = filePath;
 this.hashProvider = hashProvider;
 this.fileLength = new FileInfo(filePath).Length;
 this.memoryMappedFile = MemoryMappedFile.CreateFromFile(filePath, FileMode.Open);
 this.Checksums = this.GenerateChecksums();
 }
 
 public void Dispose()
 {
 this.memoryMappedFile.Dispose();
 if (this.hasOwnershipOfHashprovider)
 {
 this.hashProvider.Dispose();
 }
 } 

But what happens if the user of that class won't call Dispose() ? Take a look at proper-use-of-the-idisposable-interface.

• I don't see why the Read() method should be public. If it needs to be public, you should add some proper validation of the passed arguments.

• If a filePath is passed to the constructor which represents a file with Length == 0 the MemoryMappedFikle.Create() method will throw an ArgumentException which should better be thrown by your code.