I am trying to implement a tiny text file read / write class FileIO
in order to make the operations of reading / writing pure text files easily. The usage is like:
FileIO.Instance.FileWrite("TextFile1.txt", "This is a test1.");
FileIO.Instance.FileWrite("TextFile2.txt", "This is a test2.");
Console.WriteLine(FileIO.Instance.ReadTxTFile("TextFile1.txt", System.Text.Encoding.Default));
Console.WriteLine(FileIO.Instance.ReadTxTFile("TextFile2.txt", System.Text.Encoding.Default));
The experimental implementation
The experimental implementation of FileIO
class is as below.
class FileIO
{
public readonly static FileIO Instance = new FileIO();
public void FileWrite(string filename, string inputString)
{
FileStream file_stream = new FileStream(filename, FileMode.Append);
byte[] Input_data = System.Text.Encoding.Default.GetBytes(inputString);
file_stream.Write(Input_data, 0, Input_data.Length);
file_stream.Flush();
file_stream.Close();
}
public void FileWrite(string filename, string inputString, FileMode fileMode)
{
FileStream file_stream = new FileStream(filename, fileMode);
byte[] Input_data = System.Text.Encoding.Default.GetBytes(inputString);
file_stream.Write(Input_data, 0, Input_data.Length);
file_stream.Flush();
file_stream.Close();
}
public void FileWrite(string filename, string inputString, FileMode fileMode, Encoding encoding)
{
FileStream file_stream = new FileStream(filename, fileMode);
byte[] Input_data = encoding.GetBytes(inputString);
file_stream.Write(Input_data, 0, Input_data.Length);
file_stream.Flush();
file_stream.Close();
}
public string ReadTxTFile(string filename, Encoding encoding)
{
System.IO.StreamReader textreader;
string inputString;
inputString = "";
try
{
textreader = new System.IO.StreamReader(filename, encoding);
}
catch (Exception)
{
return "";
throw;
}
inputString = textreader.ReadToEnd();
textreader.Close();
return inputString;
}
}
If there is any possible improvement about:
Performance: including data reading/writing speed things
The naming and readability
Potential drawbacks of the implemented methods
, please let me know.
1 Answer 1
Naming
FileIO
: TheFile
class resides inside theSystem.IO
namespace.FileIO
is quite a misfortune chose of name.TextFileWrapper
,TextFileUtil
orTextFileManager
would be a more appropriate name for this in my opinion.FileWrite
: The general guidance is to start your methods with a verb. So,WriteFile
would be a better name for this.ReadTxtFile
: This name is a bit misleading.txt
can be considered as a file extension, so the caller may no specify the filename as you expect.inputString
: Please try to avoid Hungarian notation.
Please try to chase consistency (or symmetry).
Try to name your Read-Write method pair in the same fashion:
ReadTextFile
,WriteTextFile
ReadFile
,WriteFile
- or simply just
Read
andWrite
Stuttering:
- Maybe it's just for me, but FileIO.Instance.FileWrite seems too repetitive.
TextFileManager.Write
seems way more natural for me.
Robustness
Your implementation is quite naive. It does not deal with a lot of different cases:
- What if the
fileName
points to a non-existing file? - What if the
inputString
is enormously large? - What if one of the parameters is
null
? - What if the file has been opened by another method and has not been closed yet?
Your implementation also relies on Encoding.Default
, which is computer specific.
Which means you can end up in the following situation:
FileA.txt
has been written to the disk on MachineAFileA.txt
has been copied from MachineA to MachineB- where the default encoding is different
- MachineB could not read
FileA.txt
Thread-safety
- Your
instance
property does not make your class a singleton.- Please read thoroughly Jon Skeet's guidance.
- If you make your class a singleton you have to make sure that your operations are thread-safe.
- Reading and writing the same file from different threads are handled in a proper way.
- As pointed out by others you have to do some clean-up as well.
- To make sure that if something goes wrong then your file handles are closed properly.
Performance
- Try to take advantage of async I/O
- Try to take advantage of ArrayPool when you are calling the GetBytes method.
- It has an overload which accepts a byte array
- As always, measure, measure and measure.
File.ReadAllText
andFile.WriteAllText
? Also, you use classes that implementIDisposable
yet you don'tDispose()
of them. And why aren't you encapsulating their uses in ausing
? \$\endgroup\$Dispose()
method should be added? May I update the question? \$\endgroup\$File.ReadAllText
andFile.WriteAllText
are already exist in .NET. Thus you don't need the code you wrote. Learn what theFile
class can. \$\endgroup\$Dispose()
is needed depends on the case. The best approach is to use ausing
: stackoverflow.com/questions/17357258/… \$\endgroup\$