5
\$\begingroup\$

I'm having to process an Amazon orders report (TSV flat file from the MWS Report API) and am needing to split the report based on sales channels (I could request them separately but need to keep API request counts as low as possible) whilst editing the contents of one of the columns (mapping a virtual SKU to its real counterpart). I'm using the CsvHelper library to handle the reading and writing, and would like to handle the file a line at a time to avoid loading the entire file into memory at once, which means as far as I know I can't use the using block on the StreamWriter and CsvWriter instances, so I made a wrapper class that implements IDisposable and calls Dispose on both writers as part of its Dispose method. I store a Dictionary of these classes using sales channel as the key, then once the file has been completed I loop through the dictionary calling Dispose on each element.

Wrapper Class:

internal class AmazonCsvWriter : IDisposable
{
    private bool _disposed;
    private readonly StreamWriter _streamWriter;
    private readonly CsvWriter _csv;

    public AmazonCsvWriter(string folder)
    {
        if (!Directory.Exists(folder))
        {
            Directory.CreateDirectory(folder);
        }

        _streamWriter = new StreamWriter(Path.Combine(folder, $"{DateTime.Now:yyyyMMddHHmmss}.txt"), false, Encoding.GetEncoding("Windows-1252"));
        _csv = new CsvWriter(_streamWriter);
        _csv.Configuration.Delimiter = "\t";
        _csv.Configuration.RegisterClassMap(new AmazonOrderRowMap());
        _csv.WriteHeader<AmazonOrderRow>();
        _csv.NextRecord();
    }

    public void WriteRecord(AmazonOrderRow record)
    {
        _csv.WriteRecord(record);
        _csv.NextRecord();
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual  void Dispose(bool disposing)
    {
        if (_disposed)
        {
            return;
        }

        if (disposing)
        {
            _csv.Dispose();
            _streamWriter.Dispose();
        }

        _disposed = true;
    }
}

Used in this function:

    public void ProcessFbaOrders(string reportFile)
    {
        try
        {
            using (var sr = new StreamReader(reportFile, Encoding.GetEncoding("Windows-1252")))
            using (var csv = new CsvReader(sr))
            {
                csv.Configuration.Delimiter = "\t";
                csv.Configuration.RegisterClassMap(new AmazonOrderRowMap());
                IEnumerable<AmazonOrderRow> records = csv.GetRecords<AmazonOrderRow>();
                foreach (AmazonOrderRow record in records)
                {
                    if (!_csvWriters.ContainsKey(record.SalesChannel))
                    {
                        _csvWriters.Add(record.SalesChannel,
                            new AmazonCsvWriter(Path.Combine(_platform.OutputFolder, record.SalesChannel)));
                    }

                    record.Sku = GetRealSku(record.Sku);

                    _csvWriters[record.SalesChannel].WriteRecord(record);
                }

            }
        }
        finally
        {
            foreach (KeyValuePair<string, AmazonCsvWriter> kvp in _csvWriters)
            {
                kvp.Value.Dispose();
            }
        }

        File.Delete(reportFile);
    }

_csvWriters is defined as:

    private readonly Dictionary<string, AmazonCsvWriter> _csvWriters;

Is this safely closing the Stream/CsvWriters or is there a better method for handling this?

\$\endgroup\$
3
\$\begingroup\$

First I would like to say that your code looks clean and well structured. You are doing most things well like using the proper casing for naming your things, using using's and stacking them as well, using readonly where you should.

Improvements could be made by using better names for your variables like e.g _csv and csv. A reader of your code who is switching between the AmazonCsvWriter class and the ProcessFbaOrders() method will get confused because in the AmazonCsvWriter class the _csv variable holds an instance of a CsvWriter and in the mentioned method the csv variable holds an instance of a CsvReader.

You are using a Dictionary<string, AmazonCsvWriter> to write the the unordered records to their desired destination based on the SalesChannel of the record.

Although your code accomplish its goal you could do better by using Enumerable.GroupBy() like so

IEnumerable<IGrouping<string, AmazonOrderRow>> query =
        records.GroupBy(record => record.SalesChannel, record=> record);  

and then you can iterate over it like so

foreach (IGrouping<string, AmazonOrderRow> recordGroup in query)
{
    string csvOutputPath = Path.Combine(_platform.OutputFolder, recordGroup.Key);
    Directory.CreateDirectory(csvOutputPath);

    using (var streamWriter = new StreamWriter(Path.Combine(csvOutputPath, $"{DateTime.Now:yyyyMMddHHmmss}.txt"), false, Encoding.GetEncoding("Windows-1252")))
    using (var csvWriter= new CsvWriter(streamWriter))
    {
        csvWriter.Configuration.Delimiter = "\t";
        csvWriter.Configuration.RegisterClassMap(new AmazonOrderRowMap());
        csvWriter.WriteHeader<AmazonOrderRow>();
        csvWriter.NextRecord();
        foreach(var record in recordGroup)
        {
            record.Sku = GetRealSku(record.Sku);
            csvWriter.WriteRecord(record);
            csvWriter.NextRecord();
        }
    }
}

As a side note, you don't need to check if a directory exists before you call Directory.CreateDirectory() because the CreateDirectory() method checks the same and if the directory exists it simply returns.

\$\endgroup\$
  • \$\begingroup\$ I suspect using GroupBy results in reading the whole collection into memory. The OP is making one pass when reading and then writing and is basically holding on to only one line at a time which would be more memory efficient. Grouping would require loading everything into memory which, if the file is large, could result in unstable behavior. \$\endgroup\$ – Nkosi Dec 28 '17 at 7:21
  • \$\begingroup\$ @Nkosi you are suspecting correctly about GroupBy. Assume the file has 50000 rows with 5000 different SalesChannel this would create a Dictionary containing 5000 CsvWriter and StreamWriter objects which would consume a lot of memory as well. Without knowing the size of the file and the count of different SalesChannel I will stay with my answer. \$\endgroup\$ – Heslacher Dec 28 '17 at 7:46
  • 1
    \$\begingroup\$ @Heslacher Thanks for your answer. In regards to the sales channels (basically which Amazon TLD the order was placed through) I'm currently only looking at a max of 5 and can't see it going much higher than 10. \$\endgroup\$ – Technofrood Dec 28 '17 at 9:26
  • \$\begingroup\$ How many rows are in the file ? \$\endgroup\$ – Heslacher Dec 28 '17 at 9:27
  • 1
    \$\begingroup\$ @Heslacher My current test file is around 2000 lines, with 48 columns of data. I'm probably prematurely optimising, but with the test file the GroupBy method is using ~4MB more memory than my existing method. \$\endgroup\$ – Technofrood Dec 28 '17 at 9:33
3
\$\begingroup\$

A few notes:

  • Your class does not contain a finalizer, so there's no use in calling GC.SuppressFinalize(this).
  • The disposing foreach loop can be simplified to foreach (var writer in _csvWriters.Values) { writer.Dispose(); }.
  • You're not clearing the _csvWriters dictionary after disposing its contents.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.