CODEWITHSUNDEEP
c#
Fabio Soares
Fabio Soares

Reputation: 101

The best and right way to close StreamWriter and StreamReader

I have been trying to organize a code that it is a mess! The first and my biggest problem at this point is that one of my StreamWriters or StreamReader is being left open. Using this link, I am trying to organize my code. But my problem is that I am not sure where should I close it:

My code is:

public static void ProcessFile(string[] ProcessFile, int id_customer, string directoryinprocess)
{
    StreamWriter Writer = null, Writer2 = null, Writer3 = null;

    foreach (string filename in ProcessFile)
    {

        // Used for the output name of the file
        var dir = Path.GetDirectoryName(filename);
        var fileName = Path.GetFileNameWithoutExtension(filename);
        var ext = Path.GetExtension(filename);
        var folderbefore = Path.GetFullPath(Path.Combine(dir, @"..\"));
        int rowCount = 0;
        string path_body_out = "";
        string outputname = folderbefore + "output_temp\\" + fileName;

        if (filename.Contains("RO_"))
        {
            Writer = new StreamWriter(dir + "\\" + "output_temp\\" + fileName + "_hd_intermediate" + ext) { AutoFlush = true };
            Writer2 = new StreamWriter(dir + "\\" + "output_temp\\" + fileName + "_body_out" + ext) { AutoFlush = true };
            path_body_out = dir + "\\" + "output_temp\\" + fileName + "_hd_intermediate" + ext;
        } // end of if
        else
        {
            Writer3 = new StreamWriter(dir + "\\" + "output_temp\\" + fileName + "_out" + ext) { AutoFlush = true };
        } // end of else

        using (StreamReader Reader = new StreamReader(@filename))
        {
            while (!Reader.EndOfStream)
            {
                string inputLine = string.Empty;
                inputLine = Reader.ReadLine();

                rowCount++;

                if (filename.Contains("RO_"))
                {
                    if (rowCount <= 4)
                    {
                            Writer.WriteLine(inputLine);
                    }
                    if (rowCount >= 5)
                    {
                        Writer2.WriteLine(inputLine);
                    }
                }
                else
                {
                    { Writer3.WriteLine(inputLine); }
                }

            } // end of the while
        } // end of using Stremreader


        if (path_body_out.Contains("_hd_intermediate"))
        {
            ManipulateHeaderFilesTypeRo(dir, path_body_out);
        }
        else
        { }
    } // end of the foreach


    string[] extensions = { "_fv", "_body", "_out" };

    string[] fileEntriesout = System.IO.Directory.EnumerateFiles(directoryinprocess, "*.csv", System.IO.SearchOption.AllDirectories)
    .Where(file => extensions.Any(ex => Path.GetFileNameWithoutExtension(file).EndsWith(ex)))
        .ToArray();


    foreach (string filenameout in fileEntriesout)
    {
        string destinytablename = null;

        if (filenameout.Contains("_hd_intermediate_fv"))
        { destinytablename = "TBL_DATA_TYPE_RO_HEADER"; }
        else if (filenameout.Contains("_body_out"))
        { destinytablename = "TBL_DATA_TYPE_RO_BODY"; }
        else
        { destinytablename = "TBL_DATA_TYPE_LOAD"; }

        string id_file = Get_id_file(filenameout, id_customer);

        DataTable csvFileData = GetDataTabletFromCSVFile(filenameout, id_file);

        InsertDataIntoSQLServerUsingSQLBulkCopy(csvFileData, destinytablename);

    } // end of the foreach

    //} // end of the foreach

} // end of ProcessFile

Should I close here? 

        if (filename.Contains("RO_"))
        {
            Writer = new StreamWriter(dir + "\\" + "output_temp\\" + fileName + "_hd_intermediate" + ext) { AutoFlush = true };
            Writer2 = new StreamWriter(dir + "\\" + "output_temp\\" + fileName + "_body_out" + ext) { AutoFlush = true };
            path_body_out = dir + "\\" + "output_temp\\" + fileName + "_hd_intermediate" + ext;
        } // end of if
        else
        {
            Writer3 = new StreamWriter(dir + "\\" + "output_temp\\" + fileName + "_out" + ext) { AutoFlush = true };
        } // end of else

Or here?

                if (filename.Contains("RO_"))
                {
                    if (rowCount <= 4)
                    {
                            Writer.WriteLine(inputLine);
                    }
                    if (rowCount >= 5)
                    {
                        Writer2.WriteLine(inputLine);
                    }
                }
                else
                {
                    { Writer3.WriteLine(inputLine); }
                }

Upvotes: 1

Views: 904

Answers (2)

Cᴏʀʏ
Cᴏʀʏ

Reputation: 107606

If you can't reorganize this code so that every StreamWriter instance can be wrapped in a using(), then perhaps you can do something like this:

StreamWriter Writer = null, Writer2 = null, Writer3 = null;

try
{
    // your existing code
}
catch
{
    // Handle
}
finally
{
    if (Writer != null)
        Writer.Close();
    if (Writer2 != null)
        Writer2.Close();
    if (Writer3 != null)
        Writer3.Close();
}

This ensures that no matter what error(s) happen within the try that your writers will be closed.

In my opinion, conditionally instantiating objects is a smell and you should work on having different implementations based on filename.Contains("RO_"). You could use the strategy pattern and have different file processor interface implementations, choosing the correct one based on the filename. Each implementation would only know how to write to the locations it needs. This would allow you to correctly use a using() around each writer.

Upvotes: 5

MBurnham
MBurnham

Reputation: 381

Nomrally if you are using disposable objects, I would say use a using block. However, since you are conditionally instatiating disposable objects, I think the use of a try-finally block would be your best bet.

Declare disposable objects and intialize them to null outside of a try block.

Initialize the disposable objects to the instances you want inside of a try block. Take care not to change this reference anywhere inside of your try-block once you have created a disposable object.

Also inside of your try block, do everything you need to do with the disposable objects.

After your try block create a finally block (a catch block is optional, but you will need a finally block for this method to do its job.) and inside the finally block, check if the variables you declared to hold the disposable objects aren't null. and if they are not null, close them and make them null. 

StreamWriter writer = null;

try {
    if (condA) {
       writer = new StreamWriter("filePath1");
    } else if (condB) {
        writer = new StreamWriter("filePath2");
    } else {
        writer = new StreamWriter("filePath3");
    }

    // do things with writer

} catch (Exception ex) {

} finally {
    if (writer != null) {
        writer.close();
        writer = null;
    }
}

Upvotes: 1

Related Questions