Two similar functions for writing data to binary file using BinaryWriter

Question

I have two functions for writing data to binary file using BinaryWriter. The first function has single data object as argument and looks as below:

 // write single data object to binary file
 public void AddHeaderToIndexer(string path, RollHeader header)
 {
 bool isDone = false;
 bool error = false;
 DateTime quitTime = DateTime.Now.AddMilliseconds(500);
 while (!isDone && DateTime.Now < quitTime)
 {
 try
 {
 BinaryWriter writer;
 using (writer = new BinaryWriter
 (File.Open(path, FileMode.OpenOrCreate)))
 {
 writer.Seek((int)writer.BaseStream.Length, new SeekOrigin());
 WriteSingleHeader(writer, header);
 isDone = true;
 if (error)
 Program.logger.Write(LogLevel.Error, "Done after timeout.");
 }
 SetReadRightToFileForEveryone(path);
 }
 catch (IOException ex)
 {
 if (!error)
 Program.logger.Write(LogLevel.Error, ex.Message);
 error = true;
 }
 }
 }

The second is very similar, with only difference on accepting List of data objects and writing all List members to binary file. Actually, BinaryWriter is opened in Create unlike the first function, which uses OpenOrCreate.

 // write list of fdata objects to binary file
 public void WriteHeadersToIndexer(string path, List<RollHeader> headers)
 {
 bool isDone = false;
 bool error = false;
 DateTime quitTime = DateTime.Now.AddMilliseconds(500);
 while (!isDone && DateTime.Now < quitTime)
 {
 try
 {
 BinaryWriter writer;
 using (writer = new BinaryWriter
 (File.Open(path, FileMode.Create)))
 {
 foreach (RollHeader header in headers)
 WriteSingleHeader(writer, header);
 isDone = true;
 if (error)
 Program.logger.Write(LogLevel.Error, "Done after timeout.");
 }
 SetReadRightToFileForEveryone(path);
 }
 catch (IOException ex)
 {
 if (!error)
 Program.logger.Write(LogLevel.Error, ex.Message);
 error = true;
 }
 }
 }

I'm not able to find some elegant solution, how to separate all duplicate code, which is around the using BinaryWriter block. Also, you will maybe find more elegant solution of my error handling in this case. I'll be glad for such comments, too.

• Heslacher: Can you clarify why you need to Seek() if you write a single item but you don't need to seek for writing multiples ? In addition can you clarify why you use different FileMode's ?
• Majak: @Heslacher: I am using different FileModes and Seek() because in the first case, I write single data object to existing file, in the second case, I create new file and write all data objects from List to this file.
• Majak: @BCdotWEB: Yes, you are right, I'll fix it.

Answer 1

• Both methods are public so I would like to suggest doing proper argument validation up front which enables you to throw the correct exceptions and return early.

• Use the var type if the right hand side of an assignment makes the type obvious.

  For instance

  var isDone = false; 
  var quitTime = DateTime.Now.AddMilliseconds(500);
  

  instead of

  bool isDone = false; 
  DateTime quitTime = DateTime.Now.AddMilliseconds(500); 
  

• use braces {} although they might be optional. This makes your code less error prone and the intent of the code is more clear.

• Don't talk to Program but let Program pass any needed parts to your class and use it then. A child object should only talk to its parent by using events.

• you should pass the 500 ms as a optional parameter to that method. This will make your code more flexible in case you need to adjust the timeout.

• you should always try to code against interfaces rather than against concrete implementations. So either use IList<RollHeader> or better IEnumerable<RollHeader> as the passed in type of WriteHeadersToIndexer(). This enables you to pass for instance either an List<RollHeader> or RollHeader [] to that method. In fact each type which implements IEnumerable<T> will be good to pass.

From the comments of the question

I am using different FileModes and Seek() because in the first case, I write single data object to existing file, in the second case, I create new file and write all data objects from List to this file.

Based on the above I would like to suggest adding another method which is called from the former 2 methods. This new method will have one method argument being a FileMode value like so

private void WriteToIndexer(string path, IEnumerable<RollHeader> headers, FileMode mode, double timeOutMs = 500d)
{
 if (path == null) { throw new ArgumentNullException($"{path}"); }
 if (headers == null) { throw new ArgumentNullException($"{headers}"); }
 if (string.IsNullOrWhiteSpace(path)) { throw new ArgumentException("Parameter may not be empty", $"{path}"); }
 if (!headers.Any()) { return; }
 bool isDone = false;
 bool error = false;
 DateTime quitTime = DateTime.Now.AddMilliseconds(timeOutMs);
 while (!isDone && DateTime.Now < quitTime)
 {
 try
 {
 using (var stream = File.Open(path, mode))
 using (var writer = new BinaryWriter(stream))
 {
 writer.Seek((int)writer.BaseStream.Length, new SeekOrigin());
 foreach (RollHeader header in headers)
 {
 WriteSingleHeader(writer, header);
 }
 isDone = true;
 if (error)
 {
 Program.logger.Write(LogLevel.Error, "Done after timeout.");
 }
 }
 SetReadRightToFileForEveryone(path);
 }
 catch (IOException ex)
 {
 if (!error)
 {
 Program.logger.Write(LogLevel.Error, ex.Message);
 }
 error = true;
 }
 }
} 

this takes advantage of the string interpolation of C# 6.0.

No you can call this like so

public void AddHeaderToIndexer(string path, RollHeader header)
{
 WriteToIndexer(path, new RollHeader[] { header }, FileMode.OpenOrCreate);
}
public void WriteHeadersToIndexer(string path, IEnumerable<RollHeader> headers)
{
 WriteToIndexer(path, headers, FileMode.Create);
} 

• Majak: Thank you for your comments. I have few questions: 1. Which kind of proper argument validation do you mean in this case? 2. Using of 'var' type: I think that in case of "short-named" types, like bool or DateTime it is not necessary to loose type readability by using var. I'm using it rather in case of "long-named" types. 3. I'm using Program.Logger, which is singleton of my Logger class and I want to be able to use it directly, from Program.cs up to the last top class.
• Majak: Nice work, thanks! I have only one last question about Program.Logger from my previous comment. You say: "Don't talk to Program", but I really don't want to pass Logger instance to every class which can be candidate for using it. How to solve it in different way, rather then make it static in some low level class, like in Program.cs in my case? (I need to log something already there).
• Heslacher: You could either use some well known logger like log4net which has a static LogManager.GetLogger() method or you could extract an interface out of your implementation and either inject it into the constructor or set it as a property.